Repository navigation
refactor: example app - #5053
michalfedyna wants to merge 9 commits into
Conversation
MikitasK
left a comment
There was a problem hiding this comment.
looks awesome! good job 👍
just a few points to address before merge:
d553f1e to
803f2c7
Compare
803f2c7 to
d09e485
Compare
ab14dd0 to
a461e3e
Compare
JKobrynski
left a comment
There was a problem hiding this comment.
LGTM! @satya164 all yours
|
@michalfedyna the workflows are running, please resolve those comments that you've addressed to clean this PR up 🙏 |
|
@JKobrynski Thank you! |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical navigation-state persistence issue and additional preference, layout, and accessibility issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors the example app into a samples-first experience with searchable examples, stack navigation, and modal preferences.
Changes:
- Adds eight focused sample screens and a samples catalog.
- Replaces drawer navigation and preferences with stack navigation and a modal.
- Adds example search, preference state management, and typed object utilities.
Review findings:
- Critical (3 votes): Version or validate persisted navigation state after the navigator migration.
- Moderate (3 votes): Reset must restore the initial RTL setting.
- Moderate (2 votes): Keep the preferences action available while searching.
- Moderate (1 vote): Move
Dialog.ActionsoutsideDialog.Content. - Moderate (1 vote): Add accessible, row-specific labels to order checkboxes.
File summaries
| File | Description |
|---|---|
example/utils/typedObject.ts |
Adds typed object helpers. |
example/utils/index.ts |
Removes the obsolete web helper. |
example/src/SamplesList.tsx |
Adds the samples catalog. |
example/src/Samples/WorkspaceSample.tsx |
Adds the workspace sample. |
example/src/Samples/types.ts |
Defines sample metadata. |
example/src/Samples/SignUpSample.tsx |
Adds the sign-up sample. |
example/src/Samples/SettingsSample.tsx |
Adds the settings sample. |
example/src/Samples/PlayerSample.tsx |
Adds the player sample. |
example/src/Samples/OrdersSample.tsx |
Adds the orders sample; checkbox labels require accessibility updates. |
example/src/Samples/HelpCenterSample.tsx |
Adds the help center sample. |
example/src/Samples/ContactsSample.tsx |
Adds the contacts sample. |
example/src/Samples/ArticleSample.tsx |
Adds the article sample. |
example/src/RootNavigator.tsx |
Configures sample and example routes. |
example/src/Preferences/useSetupPreferences.ts |
Centralizes preference persistence; navigation-state migration and RTL reset require fixes. |
example/src/Preferences/usePreferences.tsx |
Adds the preferences hook. |
example/src/Preferences/PreferencesModal.tsx |
Adds the preferences modal; dialog actions need to be moved outside content. |
example/src/Preferences/PreferencesContext.tsx |
Updates the preferences context type. |
example/src/index.tsx |
Integrates navigation and preferences setup. |
example/src/Examples/TooltipExample.tsx |
Updates platform detection. |
example/src/Examples/SnackbarExample.tsx |
Uses the preferences hook. |
example/src/Examples/SearchbarExample.tsx |
Replaces drawer access with preferences access. |
example/src/Examples/CardExample.tsx |
Uses the preferences hook and platform API. |
example/src/ExampleListHeader.tsx |
Adds searchable example controls; the preferences action must remain persistent. |
example/src/ExampleList.tsx |
Adds example filtering and empty-state handling. |
example/src/DrawerItems.tsx |
Removes the legacy drawer implementation. |
Review details
Suppressed comments (2)
example/src/Preferences/PreferencesModal.tsx:164
Dialog.Actionsis nested insideDialog.Content, but Paper's dialog API expects these to be sibling children. This applies the content padding around the actions and the actions container padding again, producing incorrect spacing. Move the</Dialog.Content>before<Dialog.Actions>.
<Dialog.Actions>
<Button onPress={_handleDismissRTLDialog}>Ok</Button>
</Dialog.Actions>
</Dialog.Content>
example/src/Samples/OrdersSample.tsx:122
- Each row renders a bare checkbox without an accessible label, so assistive technology exposes multiple indistinguishable checkbox controls and users cannot tell which order they are selecting. Add a row-specific label such as
Select order ${order.id}.
<Checkbox
status={selected.includes(order.id) ? 'checked' : 'unchecked'}
onPress={() => toggleSelected(order.id)}
/>
- Files reviewed: 25/25 changed files
- Comments generated: 3
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import type { Preferences } from './PreferencesContext'; | ||
| import { dynamicThemeSupported } from '../../utils'; | ||
|
|
||
| const PERSISTENCE_KEY = 'NAVIGATION_STATE'; |
| traileringIcon="cog" | ||
| traileringIconAccessibilityLabel="preferences" | ||
| onTraileringIconPress={togglePreferences} | ||
| clearAccessibilityLabel="clear search" | ||
| autoCorrect={false} | ||
| autoCapitalize="none" | ||
| style={styles.searchbar} | ||
| /> |
| setShouldUseDynamicTheme(true); | ||
| setIsDarkMode(false); | ||
| setCustomFont(false); | ||
| setRippleEffectEnabled(true); | ||
| setPreferencesVisible(false); | ||
|
|
||
| try { | ||
| await AsyncStorage.multiRemove([PREFERENCES_KEY, PERSISTENCE_KEY]); | ||
| } catch (e) { | ||
| // ignore error | ||
| } | ||
|
|
||
| setNavigationKey((oldValue) => oldValue + 1); | ||
| }, []); |
|
Review by Claude Web
|
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
- This branch conflicts with
main, and some APIs it uses changed there. After the rebase:SearchbarrenamedtraileringIcon,traileringIconAccessibilityLabelandonTraileringIconPresstotrailingIcon,trailingIconAccessibilityLabelandonTrailingIconPress(#5117, used inExampleListHeader.tsx).ModalrenamedoverlayAccessibilityLabeltodismissAccessibilityLabeland now needs anaria-label(#5125, used inPreferencesModal.tsxandHelpCenterSample.tsx).Dialogrenders its ownPortalnow, so thePortalwrappers inPreferencesModal.tsxandSettingsSample.tsxcan go. @react-navigation/draweris still inexample/package.json, but nothing in the example imports it after this change. Remove it.
See the inline comments for the rest. (via Claude, verify if it makes sense)
| {item.components.map((component) => ( | ||
| <Chip key={component} compact> | ||
| {component} | ||
| </Chip> | ||
| ))} |
There was a problem hiding this comment.
These tags are Chips without onPress, so each one is still a button for screen readers. On Android TalkBack announces every tag as a disabled button (6 to 8 per card), and iOS and web also expose them as buttons. The card itself, which is the real control, has no button role on any platform. Render the tags as plain Text (or another non-pressable element) so only the card is interactive. Giving the card a button role may need a change in Card, because Card passes role to its outer Surface and not to the inner Pressable. (via Claude, verify if it makes sense)
| <IconButton | ||
| icon={liked ? 'heart' : 'heart-outline'} | ||
| selected={liked} | ||
| onPress={() => setLiked(!liked)} | ||
| /> | ||
| </Tooltip> | ||
| <Tooltip title={bookmarked ? 'Remove bookmark' : 'Bookmark'}> | ||
| <IconButton | ||
| icon={bookmarked ? 'bookmark' : 'bookmark-outline'} | ||
| selected={bookmarked} | ||
| onPress={() => setBookmarked(!bookmarked)} | ||
| /> |
There was a problem hiding this comment.
The like and bookmark buttons have no accessible name. iOS and Android read them as "icon-button" or "Button", and on web they are buttons with only an icon glyph as text. The Tooltip does not label them. Add an aria-label that matches the tooltip:
| <IconButton | |
| icon={liked ? 'heart' : 'heart-outline'} | |
| selected={liked} | |
| onPress={() => setLiked(!liked)} | |
| /> | |
| </Tooltip> | |
| <Tooltip title={bookmarked ? 'Remove bookmark' : 'Bookmark'}> | |
| <IconButton | |
| icon={bookmarked ? 'bookmark' : 'bookmark-outline'} | |
| selected={bookmarked} | |
| onPress={() => setBookmarked(!bookmarked)} | |
| /> | |
| <IconButton | |
| icon={liked ? 'heart' : 'heart-outline'} | |
| selected={liked} | |
| aria-label={liked ? 'Remove like' : 'Like'} | |
| onPress={() => setLiked(!liked)} | |
| /> | |
| </Tooltip> | |
| <Tooltip title={bookmarked ? 'Remove bookmark' : 'Bookmark'}> | |
| <IconButton | |
| icon={bookmarked ? 'bookmark' : 'bookmark-outline'} | |
| selected={bookmarked} | |
| aria-label={bookmarked ? 'Remove bookmark' : 'Bookmark'} | |
| onPress={() => setBookmarked(!bookmarked)} | |
| /> |
The labels match the tooltip text. (via Claude, verify if it makes sense)
| trigger={{ | ||
| icon: 'share-variant', | ||
| onPress: () => setMenuExpanded(true), | ||
| }} |
There was a problem hiding this comment.
The share FAB has no accessible name either. iOS and Android expose it as "fab-shell" and web as an unlabelled button. trigger accepts aria-label:
| trigger={{ | |
| icon: 'share-variant', | |
| onPress: () => setMenuExpanded(true), | |
| }} | |
| trigger={{ | |
| icon: 'share-variant', | |
| 'aria-label': 'Share', | |
| onPress: () => setMenuExpanded(true), | |
| }} |
The same applies to any icon-only control in the samples. (via Claude, verify if it makes sense)
| </List.Section> | ||
| </List.AccordionGroup> | ||
|
|
||
| <TouchableRipple onPress={() => setContactVisible(true)}> |
There was a problem hiding this comment.
"Still stuck? Contact support" opens a modal but has no button role. iOS and Android expose it as a plain group and web as a focusable div without a role, so screen reader users do not know it is a control.
| <TouchableRipple onPress={() => setContactVisible(true)}> | |
| <TouchableRipple role="button" onPress={() => setContactVisible(true)}> |
The ripple then announces as "Still stuck? Contact support, button". (via Claude, verify if it makes sense)
| <Appbar.Header> | ||
| <Appbar.BackAction onPress={() => navigation.goBack()} /> | ||
| <Appbar.Content title="Workspace" /> | ||
| <Appbar.Action aria-label="magnify" icon="magnify" onPress={() => {}} /> |
There was a problem hiding this comment.
aria-label="magnify" is the icon name, so screen readers announce "magnify". Use the action instead:
| <Appbar.Action aria-label="magnify" icon="magnify" onPress={() => {}} /> | |
| <Appbar.Action aria-label="Search" icon="magnify" onPress={() => {}} /> |
The label then describes what the button does. (via Claude, verify if it makes sense)
| }, | ||
| fab: { | ||
| position: 'absolute', | ||
| right: 16, |
There was a problem hiding this comment.
With RTL on, iOS and Android move this FAB to the left, but on web it stays on the right because right is a physical edge there. end maps to the end edge on all three platforms.
| right: 16, | |
| end: 16, |
On native this keeps the current position, since right already swaps there. (via Claude, verify if it makes sense)
| <ToggleButton.Row | ||
| value={repeat} | ||
| onValueChange={(value) => value && setRepeat(value)} | ||
| > | ||
| <ToggleButton icon="repeat-off" value="off" /> | ||
| <ToggleButton icon="repeat-once" value="one" /> | ||
| <ToggleButton icon="repeat" value="all" /> | ||
| </ToggleButton.Row> |
There was a problem hiding this comment.
ToggleButton was removed on main in #5095, so this sample breaks on rebase. The three repeat buttons also have no accessible name: iOS and Android read each one as "icon-button". SegmentedButtons with icon and aria-label per item (or IconButton with selected and aria-label, as TooltipExample does on main) covers the same case. Update components in PlayerSampleConfig to match. (via Claude, verify if it makes sense)
| const [initialRtl] = React.useState(getInitialRtl); | ||
|
|
||
| const [shouldUseDynamicTheme, setShouldUseDynamicTheme] = | ||
| React.useState(true); | ||
| const [isDarkMode, setIsDarkMode] = React.useState(false); | ||
| const [rtl, setRtl] = React.useState(initialRtl); |
There was a problem hiding this comment.
initialRtl is only used to seed rtl, so the extra state adds nothing:
| const [initialRtl] = React.useState(getInitialRtl); | |
| const [shouldUseDynamicTheme, setShouldUseDynamicTheme] = | |
| React.useState(true); | |
| const [isDarkMode, setIsDarkMode] = React.useState(false); | |
| const [rtl, setRtl] = React.useState(initialRtl); | |
| const [shouldUseDynamicTheme, setShouldUseDynamicTheme] = | |
| React.useState(true); | |
| const [isDarkMode, setIsDarkMode] = React.useState(false); | |
| const [rtl, setRtl] = React.useState(getInitialRtl); |
getInitialRtl still runs once, on the first render. (via Claude, verify if it makes sense)
| modalContent: { | ||
| marginHorizontal: 24, | ||
| }, |
There was a problem hiding this comment.
| }, | ||
| }} | ||
| persistor={navigationPersistor} | ||
| linking={{ config: { initialRouteName: 'SamplesList' } }} |
There was a problem hiding this comment.
With initialRouteName: 'SamplesList', a URL like /button builds the stack [SamplesList, Button]. After a page reload on web (or a deep link) on any example, Back goes to Samples and not to the Examples list, which is where it went on main. If examples should keep returning to the list, put ExampleList and the example screens in a nested stack whose initialRouteName is ExampleList. (via Claude, verify if it makes sense)




Motivation
PR improves example app. It adds:
Related issue
#4992
Test plan
yarn lint,yarn typecheck,yarn testand a visual check of provided screenshots