-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$250] Copilot - Two accounts are highlighted in account list when navigating via keyboard #50779
Comments
Triggered auto assignment to @mjasikowski ( |
Triggered auto assignment to @greg-schroeder ( |
💬 A slack conversation has been started in #expensify-open-source |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
Account switcher component was changed in this PR: #49051 - this is most likely the offender |
@mjasikowski In this PR, we discussed this and agreed it isn't a problem. |
@Expensify/design can you weigh in here and let us know if this is OK or expected? I personally have seen this behavior outside of NewApp and I'm used to it in terms of UX, so it doesn't bother me - but nevertheless we need to either change this or readjust QA to take the new keyboard navigation into account when doing tests. |
Currently, this is the behavior of other Screen.Recording.2024-10-15.at.15.46.30.mov |
Interesting, I think I would expect the keyboard navigation to use the same background color as hover. So this way we save the selected color for when a row is truly selected/checked, and we just reuse a background hover highlight for keyboard nav. Thoughts on something like that? |
We were aware of this during PR review and the bug is not new, it's an existing bug in Screen.Recording.2024-10-15.at.10.41.12.AM.movScreen.Recording.2024-10-15.at.10.42.02.AM.mov |
OK, removing the deployblocker tag for now and switching this to weekly |
Agree with this. I feel like this is the most common pattern I see for this type of thing. |
Does that mean we'd have to change the behaviour on other lists i.e. the ones that use |
Personally I think we should. IMO selected items should always have a background and the hover color makes more sense for keyboard navigating. Just because something is focused with the keyboard doesn't mean it's selected, so I think keyboard navigating is more similar to hovering with a mouse cursor than it is having the item selected. |
Yup, I agree with Danny here |
Got it 👍 |
@greg-schroeder Let's make this |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee ( |
@jjcoffee I can take this over as C+ if you want |
Edited by proposal-police: This proposal was edited at 2024-10-15 19:22:02 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Both the current account and the selected account are highlighted when navigating via keyboard. What is the root cause of that problem?When an item is focused, the background color is calculated by App/src/components/MenuItem.tsx Lines 586 to 588 in 4f1224c
In App/src/components/PopoverMenu.tsx Line 322 in 4f1224c
So the selected item and the focused item by keyboard has the same background color What changes do you think we should make in order to solve the problem?We can pass
App/src/components/PopoverMenu.tsx Line 322 in 4f1224c
Or if we don't want to remove
We should check for all the places we're using
Line 1214 in e8a8c53
What alternative solutions did you explore? (Optional)NA ResultScreen.Recording.2024-10-16.at.01.15.12.mov |
@shawnborton Is the resulting video in my proposal the expected behavior we want? |
@nkdengineer Let's fix the inconsistency with the other lists too e.g. |
Updated proposal to include the other places. |
That looks correct to me. |
Yup agree with Danny, that looks like what I had in mind. |
@nkdengineer's proposal LGTM! I'm a little unsure about blanket-changing this in other areas (the For auto-suggest-2024-10-16_11.43.37.mp4I'm unsure about changing how this works in search too, as this feels pretty natural that there's a distinct hover effect from the mouse, and then the keyboard navigation is changing the "selected" item (i.e. that will be chosen on pressing enter). search-highlights-2024-10-16_11.44.14.mp4I wonder if we should focus on fixing the most obvious problem with 🎀👀🎀 C+ reviewed |
Current assignee @mjasikowski is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
@s77rt Sorry I have literally zero work at the moment, so I can't really pass on this one 😅 |
@jjcoffee I'd like for @Expensify/design to weigh in, but I don't really see a difference between hovering with your cursor and navigating through the choices with your keyboard. In the examples you showed, nothing has actually been selected yet, so it still makes sense to me to use the hover background color. I think as long as there is a clear indicator that you have something "focused" while navigating/hovering, it's fine. But maybe I'm alone in this feeling! |
I don't have anything else to add, I agree with Danny - again that was my original feedback about this and I think we should have used this style for this interaction all along. |
Does anyone have thoughts on which project board this live under? |
Agree. Same intent, different input device. |
Great, thanks for confirming the expected behaviour all. Just waiting for @mjasikowski to assign @nkdengineer then! |
📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR review continues |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.49-0
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh081006@applause.expensifail.com
Issue reported by: Applause Interna Team
Action Performed:
Expected Result:
The current account will no longer be highlighted and the selected account will be highlighted (same behavior as workspace switcher).
Actual Result:
Both the current account and the selected account are highlighted when navigating via keyboard.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6634911_1728972462581.20241015_140418.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @jjcoffeeThe text was updated successfully, but these errors were encountered: