Migrate YearPickerModal to a @react-navigation modal screen#91106
Migrate YearPickerModal to a @react-navigation modal screen#91106trasnake87 wants to merge 2 commits into
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
Bot
requested review from
flaviadefaria and
parasharrajat
and removed request for
a team
|
@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9781e56e2a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| entryScreens: [ | ||
| SCREENS.SETTINGS.PROFILE.DATE_OF_BIRTH, | ||
| SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_DATE, | ||
| SCREENS.MONEY_REQUEST.STEP_DATE, | ||
| SCREENS.MONEY_REQUEST.STEP_TIME, | ||
| SCREENS.MONEY_REQUEST.STEP_TIME_EDIT, | ||
| SCREENS.MONEY_REQUEST.SPLIT_EXPENSE_CREATE_DATE_RANGE, | ||
| SCREENS.SEARCH.ROOT, | ||
| SCREENS.SEARCH.ADVANCED_FILTERS_DATE_RHP, | ||
| SCREENS.SEARCH.EDIT_MULTIPLE_DATE_RHP, | ||
| SCREENS.CHRONOS_SCHEDULE_OOO_ROOT, | ||
| SCREENS.SCHEDULE_CALL, | ||
| ], |
There was a problem hiding this comment.
CalendarPicker now always opens the year chooser through a dynamic route, and getStateFromPath() only authorizes that route when the current screen is in DYNAMIC_ROUTES.YEAR_SELECTOR.entryScreens; otherwise it falls back as an unauthorized dynamic route. The new allowlist here omits several screens that still render DatePicker (SCREENS.SETTINGS.WALLET.PERSONAL_CARD_EDIT_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARDS_ASSIGN_CARD_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARD_EDIT_TRANSACTION_START_DATE, and SCREENS.DEBUG.DYNAMIC_DETAILS_DATE_TIME_PICKER_PAGE), so tapping the calendar year on those pages will not navigate to the new year selector.
Useful? React with 👍 / 👎.
|
Good catch on the entry screens. Rather than extend the allowlist, I switched Reasoning: Added a regression test in |
trasnake87
force-pushed
the
yearpicker-rnav-90469
branch
from
83b38b6 to
2a4c5ce
Compare
Replace the react-native-modal based YearPickerModal, rendered inside CalendarPicker, with a dynamic @react-navigation route so it animates consistently with the surrounding native stack. - Add the DYNAMIC_YEAR_SELECTOR dynamic route and the standalone DynamicYearSelectorPage screen (year list + search), built from route query params. - Lift the picker result out of CalendarPicker local state into a transient Onyx key, tagged with a per-instance contextID so the correct CalendarPicker consumes it (range pickers mount two). - CalendarPicker navigates to the year route and applies the returned year; hosts that render it inside a popover pass onBeforeOpenYearPicker to dismiss the popover before the year screen is shown. - Remove YearPickerModal and the now-unused year list state. - Update CalendarPicker tests for the navigation flow and add coverage for the year selection action.
trasnake87
force-pushed
the
yearpicker-rnav-90469
branch
from
2a4c5ce to
364652d
Compare
CalendarPicker is a generic component reached from many screens (date input fields, DateSelectPopup, RangeDatePicker, DatePresetFilterBase, ScheduleCallPage, ...). The previous in-place YearPickerModal had no screen restriction, so a hand-maintained entryScreens allowlist silently broke the year picker on omitted screens (e.g. company/personal card transaction start date pages, debug datetime picker). Use the '*' wildcard so the year selector stays reachable from every CalendarPicker host and does not regress when new date-input screens are added. Added a regression test asserting the route remains unrestricted.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
trasnake87
force-pushed
the
yearpicker-rnav-90469
branch
from
364652d to
883930e
Compare
|
Fix the checklist @trasnake87 and add vids for all platforms. |
|
Added the Android: mWeb Chrome video and ticked the offline-steps box (the section was already in the body, just hadn't been checked). iOS to follow. |
2 participants
Explanation of Change
YearPickerModalwas areact-native-modal(RIGHT_DOCKED) rendered insideCalendarPicker, driven by localisYearPickerVisible/currentDateViewstate. Becausereact-native-modalmounts and animates independently of the native stack, it animated inconsistently with the surrounding@react-navigationscreens (the problem described in the parent migration #53493).This migrates the year picker to a dynamic
@react-navigationmodal route, following the pattern established by PR #88915 (DynamicExpenseLimitTypeSelectorPage):DYNAMIC_ROUTES.YEAR_SELECTORand a standaloneDynamicYearSelectorPagescreen (the year list + search input, built from route query params), wired throughROUTES,SCREENS,ModalStackNavigators,linkingConfig, and navigationtypes.CalendarPicker's local state into a transient Onyx key (CALENDAR_PICKER_SELECTED_YEAR), tagged with a per-instancecontextID.CalendarPickeris rendered in multiple places, andRangeDatePickermounts two instances simultaneously, so the result is matched back to the instance that opened the picker viacontextIDand cleared once consumed.CalendarPickernow navigates to the year route (viacreateDynamicRoute) and applies the returned year to the displayed date. Hosts that renderCalendarPickerinside a popover (DatePickerModal, the Search date dropdown) passonBeforeOpenYearPickerso the popover is dismissed before the year screen is shown, instead of stacking a screen behind areact-native-modal.YearPickerModaland the now-unused year-list state inCalendarPicker.Fixed Issues
$ #90469
PROPOSAL: #90469 (comment)
Tests
Offline tests
The year selector is client-side navigation and writes only to a transient local Onyx value; behavior is identical offline.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
and_91106_trim.mp4
Android: mWeb Chrome
web_91106_mweb_20260521_040950.mp4
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web_91106_20260519_202020.mp4