-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(appbar): make back icon respect the icon from Provider settings #5064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| import { Animated } from 'react-native'; | ||
| import { Animated, Platform, Text as RNText } from 'react-native'; | ||
|
|
||
| import { describe, expect, it, jest } from '@jest/globals'; | ||
| import { act } from '@testing-library/react-native'; | ||
| import { afterEach, describe, expect, it, jest } from '@jest/globals'; | ||
| import { act, render as rtlRender } from '@testing-library/react-native'; | ||
| import { SafeAreaProvider } from 'react-native-safe-area-context'; | ||
|
|
||
| import PaperProvider from '../../../core/PaperProvider'; | ||
| import { getTheme } from '../../../core/theming'; | ||
| import { render, screen } from '../../../test-utils'; | ||
| import { tokens } from '../../../theme/tokens'; | ||
|
|
@@ -14,6 +15,7 @@ import { | |
| modeTextVariant, | ||
| renderAppbarContent as utilRenderAppbarContent, | ||
| } from '../../Appbar/utils'; | ||
| import type { IconProps } from '../../MaterialCommunityIcon'; | ||
| import Menu from '../../Menu/Menu'; | ||
| import Searchbar from '../../Searchbar'; | ||
| import Text from '../../Typography/Text'; | ||
|
|
@@ -453,3 +455,70 @@ describe('animated value changes correctly', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Appbar.BackAction icon', () => { | ||
| const originalPlatform = Platform.OS; | ||
|
|
||
| afterEach(() => { | ||
| Platform.OS = originalPlatform; | ||
| }); | ||
|
|
||
| const CustomIcon = ({ name, size, direction, testID }: IconProps) => ( | ||
| <RNText | ||
| testID={testID} | ||
| style={{ | ||
| fontSize: size, | ||
| transform: [{ scaleX: direction === 'rtl' ? -1 : 1 }], | ||
| }} | ||
| > | ||
| {`custom-${name}`} | ||
| </RNText> | ||
| ); | ||
|
|
||
| const renderBackAction = (direction?: 'ltr' | 'rtl') => | ||
| rtlRender( | ||
| <PaperProvider settings={{ icon: CustomIcon }} direction={direction}> | ||
| <Appbar.BackAction onPress={() => {}} testID="back-action" /> | ||
| </PaperProvider> | ||
| ); | ||
|
|
||
| it('renders the icon provided through PaperProvider settings', async () => { | ||
| Platform.OS = 'android'; | ||
|
|
||
| await renderBackAction(); | ||
|
|
||
| expect( | ||
| screen.getByText('custom-arrow-left', { includeHiddenElements: true }) | ||
| ).toBeOnTheScreen(); | ||
| }); | ||
|
|
||
| it('renders the icon provided through PaperProvider settings on iOS', async () => { | ||
| Platform.OS = 'ios'; | ||
|
|
||
| await renderBackAction(); | ||
|
|
||
| expect( | ||
| screen.getByText('custom-arrow-left', { includeHiddenElements: true }) | ||
| ).toBeOnTheScreen(); | ||
| }); | ||
|
|
||
| it('keeps the icon mirrored in RTL', async () => { | ||
| Platform.OS = 'android'; | ||
|
|
||
| await renderBackAction('rtl'); | ||
|
|
||
| expect( | ||
| screen.getByText('custom-arrow-left', { includeHiddenElements: true }) | ||
| ).toHaveStyle({ transform: [{ scaleX: -1 }] }); | ||
| }); | ||
|
|
||
| it('keeps the icon unmirrored in LTR', async () => { | ||
| Platform.OS = 'android'; | ||
|
|
||
| await renderBackAction('ltr'); | ||
|
|
||
| expect( | ||
| screen.getByText('custom-arrow-left', { includeHiddenElements: true }) | ||
| ).toHaveStyle({ transform: [{ scaleX: 1 }] }); | ||
| }); | ||
| }); | ||
|
Comment on lines
+485
to
+524
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we also add an iOS case alongside the existing tests? smth like that: it('renders the icon provided through PaperProvider settings on iOS', async () => {
Platform.OS = 'ios';
await renderBackAction();
expect(
screen.getByText('custom-arrow-left', { includeHiddenElements: true })
).toBeOnTheScreen();
});
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, as written. Appbar.test.tsx goes from 33 to 34 tests. Counterfactual: keeping the test and reverting |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we also respect configured icon renderer on iOS while preserving bundled chevron as the default?
Currently,
Platform.OS === 'ios'always returnsback-chevron.pngsoPaperProvider settings={{ icon: CustomIcon }}remains ignored on iOSso maybe we can do smth like that:
this should retain existing iOS appearance with default settings while satisfying reported expectation that components use the icon renderer configured through
PaperProviderThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Applied close to your sketch, with two adjustments after looking at the surrounding code.
I read settings via
React.useContext<Settings>(SettingsContext)to match howTouchableRippledoes it (src/components/TouchableRipple/TouchableRipple.tsx:128).I kept the
!icon ||half of the guard, it is not dead.PaperProviderbuilds the value as{ icon: MaterialCommunityIcon, rippleEffectEnabled: true, ...settings }(src/core/PaperProvider.tsx:51-58), so a caller passingsettings={{ icon: undefined }}spreadsiconback toundefinedand the identity check alone would miss it.The identity comparison does hold, which was the part of the sketch I was least sure about:
SettingsContext's own default (src/core/settings.tsx:17-20) andPaperProvider's default both point at the sameMaterialCommunityIconmodule export, so an app that configures nothing still gets the bundled chevron on iOS.