Skip to content

fix(appbar): make back icon respect the icon from Provider settings - #5064

Open
giaBaoJS wants to merge 1 commit into
callstack:mainfrom
giaBaoJS:fix/appbar-back-icon-respects-settings-icon
Open

fix(appbar): make back icon respect the icon from Provider settings#5064
giaBaoJS wants to merge 1 commit into
callstack:mainfrom
giaBaoJS:fix/appbar-back-icon-respects-settings-icon

Conversation

@giaBaoJS

Copy link
Copy Markdown

Motivation

Icon is the only component that reads the icon entry from SettingsContext:

}elseif(typeofs==='string'){
return(
<SettingsConsumer>
{({ icon })=>{
returnicon?.({
name: s,
color: iconColor,
size,
direction,
testID,
});
}}
</SettingsConsumer>
);

A number of components import MaterialCommunityIcon directly instead, which bypasses that consumer. Because src/core/settings.tsx#L18 also uses MaterialCommunityIcon as the default context value, those call sites look correct until an app actually passes settings={{ icon }} to PaperProvider — at which point the override is silently ignored.

Appbar.BackAction is one of the two components named in #4760.

Scope

Deliberately narrow: this PR only changes Appbar/AppbarBackIcon.tsx.

The same bypass exists at these call sites, which I left alone because they all sit under active/competing PRs and are better done separately (happy to follow up):

  • Chip/Chip.tsx (2)
  • Snackbar.tsx (1)
  • List/ListAccordion.tsx (1)
  • DataTable/DataTablePagination.tsx (4)
  • DataTable/DataTableTitle.tsx (1)
  • RadioButton/RadioButtonIOS.tsx (1)
  • Searchbar.tsx (2)

Also out of scope on purpose: the Platform.OS === 'ios' branch of AppbarBackIcon renders the bundled back-chevron.png asset rather than an icon-font glyph. Routing that through settings.icon would replace the iOS chevron with an arrow-left glyph for every app that does not customise icons, which is a visual change I did not want to smuggle into a bug fix. Let me know if you'd like it changed and I'll do it here or in a follow-up.

Note on RTL

The object form { source: 'arrow-left', direction } is used rather than the bare string "arrow-left". Icon computes direction as null for a bare string source, which drops the scaleX: -1 mirroring that the previous direction={direction} prop provided — the back arrow is direction-sensitive, so that would have been a regression. There is a dedicated test for both RTL and LTR to keep it that way.

Test plan

Added three cases to src/components/__tests__/Appbar/Appbar.test.tsx (Platform.OS is forced to android because the jest preset defaults to ios, where this branch is not reachable):

  • the icon from PaperProvider settings={{ icon }} is rendered
  • it stays mirrored in RTL
  • it stays unmirrored in LTR

Before the fix all three fail (the default arrow-left glyph renders instead of the custom one). With the bare-string form, the RTL case fails on its own.

yarn test: 55 suites, 738 passed / 1 skipped, 169 snapshots — no snapshot churn. yarn lint and yarn typecheck clean.

Fixes part of #4760.

Copy link
Copy Markdown

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 returns back-chevron.png so PaperProvider settings={{ icon: CustomIcon }} remains ignored on iOS

so maybe we can do smth like that:

const{ direction }=useLocale();const{ icon }=React.useContext(SettingsContext);constshouldUseIOSAsset=Platform.OS==='ios'&&(!icon||icon===MaterialCommunityIcon);returnshouldUseIOSAsset ? (// existing bundled iOS chevron) : (<Iconsource={{source: 'arrow-left', direction }}color={color}size={size}/>);

this should retain existing iOS appearance with default settings while satisfying reported expectation that components use the icon renderer configured through PaperProvider

Comment on lines +485 to +514
it('renders the icon provided through PaperProvider settings', async () => {
Platform.OS = 'android';

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 }] });
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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';awaitrenderBackAction();expect(screen.getByText('custom-arrow-left',{includeHiddenElements: true})).toBeOnTheScreen();});

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@giaBaoJS@MikitasK