List upcoming years ascending after the pinned year in the year picker - #102694
Conversation
Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
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
added
the
Melvin-Test-Web
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Android test steps verification results: ✅ pass On Android, the year picker shows the selected year 2026 first, then upcoming years ascending to 2126, then past years nearest first (2025, 2024, …); search and year selection also work with no errors. The PR's Tests section is still empty, so these steps come from the linked issue and the PR description.
|
|
🤖 Web test steps verification results: On Web, the year picker shows the selected year 2026 first, then upcoming years ascending, and search and year selection work with no UI errors. The 2126 → 2025 boundary could not be scrolled to on Web, so step 4 rests on the search results and the PR's sort code rather than a screenshot. It was already confirmed on screen in the Android run. The JS console could not be captured.
|
|
@MelvinBot Check all checkboxes on the PR Author Checklist including the 2 |
|
I updated the PR body, but I couldn't mark the PR ready for review. My What changed in the PR body:
Before you request review, note these gaps:
|
|
@MelvinBot If the ESLint CI fail is caused by your PR changes, fix it. Then mark the PR ready for review since I don't have permissions for that - or tag the CME assigned on the issue to help open the PR for review. |
…UpcomingYears Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
The ESLint failure wasn't caused by this PR. All 4 errors were in files the PR doesn't touch, and they came from I merged @francoisl, could you mark this PR ready for review? Neither The 4 errors from the failed run
Since this branch was created, |
|
No code fix is needed. The checks on the latest commit (
|
|
@ikevin127 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] |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-01.at.16.47.19.mov |
| const sortedYears = [...years].sort((a, b) => b.value - a.value); | ||
| // Long lists (where the pin applies) show upcoming years ascending, then past years nearest first, so the row after the pinned year is the next year. | ||
| // Short lists aren't pinned, so they keep the newest-first order. | ||
| const shouldSortAroundInitialYear = years.length >= CONST.STANDARD_LIST_ITEM_LIMIT; |
There was a problem hiding this comment.
src/components/DatePicker/CalendarPicker/YearPickerModal.tsx:44
🟢 years.length >= CONST.STANDARD_LIST_ITEM_LIMIT repeats the guard that already lives inside moveInitialSelectionToTop. If that threshold ever changes in SelectionListOrderUtils, the two can drift.
A 12 item list could then get sorted around the initial year without being pinned, so the selected year would sit in the middle (2021, 2022, ..., 2020, 2019).
Can we export the check from the util and reuse it ? Then pick the comparator once instead of re-checking the flag on every comparison:
// SelectionListOrderUtils.ts
function shouldMoveInitialSelectionToTop(itemCount: number) {
return itemCount >= CONST.STANDARD_LIST_ITEM_LIMIT;
}// YearPickerModal.tsx
const compareNewestFirst = (a: CalendarPickerListItem, b: CalendarPickerListItem) => b.value - a.value;
const compareAroundInitialYear = (a: CalendarPickerListItem, b: CalendarPickerListItem) => {
const isAUpcoming = a.value > initialYear;
const isBUpcoming = b.value > initialYear;
if (isAUpcoming !== isBUpcoming) {
return isAUpcoming ? -1 : 1;
}
return isAUpcoming ? a.value - b.value : b.value - a.value;
};
const sortedYears = [...years].sort(shouldMoveInitialSelectionToTop(years.length) ? compareAroundInitialYear : compareNewestFirst);There was a problem hiding this comment.
Done in 6b6f78c. SelectionListOrderUtils now exports shouldMoveInitialSelectionToTop, and moveInitialSelectionToTop uses it for its own guard. YearPickerModal picks compareAroundInitialYear or compareNewestFirst once from that helper, so the sort and the pin can't drift. I also added a unit test for the helper's inclusive boundary.
|
|
||
| expect(getSelectionListProps()?.data.at(0)?.value).toBe(2020); | ||
| // Then 2020 stays first and the upcoming years follow in ascending order | ||
| expect(getSelectionListProps()?.data.map((year) => year.value)).toEqual([2020, 2021, 2022, 2023, 2024, 2025, 2026, 2027]); |
There was a problem hiding this comment.
tests/ui/YearPickerModalTest.tsx:108
🟡 With currentYear={2020}, every "202" match is an upcoming year. So this test never checks that past years come after the upcoming ones in search results, which is the exact case in step 6 of the PR tests (2026, 2027, 2028, 2029, 2025, ...). Can we select a year in the middle of the matches ?
// Given 2024 selected, so "202" matches years on both sides of it
currentYear={2024}
...
expect(getSelectionListProps()?.data.map((year) => year.value)).toEqual([2024, 2025, 2026, 2027, 2023, 2022, 2021, 2020]);
| const values = getSelectionListProps()?.data.map((year) => year.value); | ||
|
|
||
| // Then the row after the pinned year is the next year, not the latest year in the list | ||
| expect(values).toEqual([2020, 2021, 2022, 2023, 2024, 2025, 2026, 2027, 2019, 2018, 2017, 2016, 2015, 2014]); |
There was a problem hiding this comment.
tests/ui/YearPickerModalTest.tsx:88
🟢 Can we add a case for each end of the list ? When the selected year is the newest year there are no upcoming years, and when it is the oldest there are no past years.
Both hit the isAUpcoming !== isBUpcoming branch differently and are easy to break later:
it.each([
[2027, [2027, 2026, 2025, 2024, 2023, 2022, 2021, 2020, 2019, 2018, 2017, 2016, 2015, 2014]],
[2014, [2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022, 2023, 2024, 2025, 2026, 2027]],
])('orders correctly when %i is at the edge of the list', (currentYear, expected) => { ... });
|
@MelvinBot Address the 3 code comments above. |
…ordering Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
@ikevin127 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] |
|
I addressed all 3 review comments in 6b6f78c and replied in each thread:
|
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Tests well and all comments were addressed
|
🚧 francoisl has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
5 participants
Explanation of Change
The year picker pins the selected year to the top of long lists, which is intended. But the other years were sorted newest-first, so the row right after 2026 was 2126 (the list runs from current year − 100 to current year + 100).
This PR keeps the pin and changes only how the remaining years are sorted in
YearPickerModal, when the pin applies (years.length >= CONST.STANDARD_LIST_ITEM_LIMIT):Short lists aren't pinned, so they keep the newest-first order. Search still keeps the selected year first.
tests/ui/YearPickerModalTest.tsxnow asserts the full order, the order within search results, and the unchanged short-list order.AI tests run:
YearPickerModalTest,CalendarPickerTest, andSelectionListOrderUtilsTestpass (46 tests). Lint,npm run typecheck, the React Compiler compliance check, and cspell pass on the changed files. An automated web test couldn't run because the test environment failed to sign in.Fixed Issues
$ #102474
PROPOSAL: #102474 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari