Fix/73489 - Two dates highlighted when scrolling date list with keyboard arrows on Freq list#74306
Fix/73489 - Two dates highlighted when scrolling date list with keyboard arrows on Freq list#74306blimpich merged 23 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
linhvovan29546 and
trjExpensify
and removed request for
a team
|
@linhvovan29546 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Show resolved
Hide resolved
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
|
Review complete! Found 2 performance-related issues that should be addressed. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Makes sense! 👍
P.s Can you try to make your PR titles more reflective of what changes the PR is making, and fill out the explanation of changes section? Thanks!
dmkt9
changed the title
Thanks! I've just updated the title and description for this one. |
#73489 (comment) Screen.Recording.2025-11-06.at.14.53.08.mov |
|
Bug: After scrolling with the keyboard arrows, if you try to scroll the page using the mouse (I’m using an external mouse), the hovered item is no longer focused. Screen.Recording.2025-11-06.at.15.39.12.mov |
| const handleMouseLeave = (e: React.MouseEvent<Element, MouseEvent>) => { | ||
| const handleMouseLeave = () => { | ||
| bind.onMouseLeave(); | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
Not sure, but I think removing this one might cause a regression.
There was a problem hiding this comment.
I’ve added a prop passed down from BaseSelectionListWithSections. By default, we call stopPropagation inside BaseListItem, but for BaseSelectionListWithSections, the propagation is stopped at the View wrapper instead.
| </> | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Can we split this into platform-specific files, like index.ts and index.native.ts, so we don’t need to check the platform manually?
There was a problem hiding this comment.
Resolved — I've pushed it into SelectionListWithSections/index.tsx
|
|
||
| let lastClientX = 0; | ||
| let lastClientY = 0; | ||
| const handler = (event: MouseEvent) => { |
There was a problem hiding this comment.
Would it be possible to move the handler outside the useEffect?
There was a problem hiding this comment.
I think keeping it inside useEffect is more efficient, since we don't use that handler elsewhere
|
I'm asking internal whether we should simply disable this ESLint rule instead of applying the fix in f50a560 |
This reverts commit f50a560.
I mean we should disable the ESLint rule no-restricted-imports too =)) |
Ah, haha. I misunderstood — I assumed we were skipping that check. It's ready again. |
|
@dmkt9 can we make so that code coverage isn't reduced? See #74306 (comment) It looks you've already written some great tests, but can we increase it just a bit to make that ❌ into a ✅? |
The tests have been added. This is now ready for review. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.2.62-0 🚀
|
|
hello @dmkt9 , I've been working on the new |
|
Hi @zfurtak, I modified |
|
Okay thank you, I understand @linhvovan29546 motivation but while I'm working on the new component, I'm trying very hard to limit the number of props and the component's complexity. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.62-5 🚀
|
|
@dmkt9 Seems like some unit test are failing on main, could you please check. |
| testID, | ||
| shouldUseDefaultRightHandSideCheckmark = true, | ||
| shouldHighlightSelectedItem = true, | ||
| shouldDisableHoverStyle, |
There was a problem hiding this comment.
Checklist from #79395:
We need to pass shouldDisableHoverStyle down to UserListItem as well. Without passing it, we see two highlighted states when using keyboard arrow navigation.
7 participants
Explanation of Change
When using the arrow up/down keys to navigate items, the hover style is temporarily disabled to prevent multiple items from appearing highlighted at the same time.
The next item to be focused when pressing the arrow key will be the currently hovered item (similar to Slack's behavior). If no item is being hovered, the focus will move to the item before or after the currently highlighted one.
Fixed Issues
$ #73489
PROPOSAL: #73489 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Test 1:
Test 2:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Test 1:
Android: Native
This issue doesn't occur
Android: mWeb Chrome
This issue doesn't occur
iOS: Native
This issue doesn't occur
iOS: mWeb Safari
This issue doesn't occur
MacOS: Chrome / Safari
mac.safari.mp4
MacOS: Desktop
mac.desktop.mp4
Test 2:
2025-11-13.16-59-13.mp4