-
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
[HOLD for payment 2024-02-20] [$250] [Wave 8] [Ideal Nav] Workspace Switcher Icon: Missing tool tip and wrong icon color #35618
Comments
Triggered auto assignment to @Christinadobrzyn ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The tooltip doesn't appear. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)NA |
Also, want to clarify that for the small button over the workspace icon, just the button BG should be changing, not the icon color. We should be following our default button styles here. |
Job added to Upwork: https://www.upwork.com/jobs/~01566d00a9005cba1a |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
Upwork job price has been updated to $250 |
@shawnborton could you confirm that the tooltip over the Workspace Switcher icon should display the full workspace name? |
Hmm I would think the tool tip should actually say "Choose a workspace" since tapping this element takes you to that flow of the app. cc @trjExpensify @Expensify/design for a gut check there though. |
Totally agree. |
Yup, I also agree with that. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Missing tool tip and wrong icon color What is the root cause of that problem?We don't wrap App/src/components/WorkspaceSwitcherButton.tsx Lines 40 to 41 in d9fd412
For the design, we define hover color change for the Icon rather than the
App/src/components/SubscriptAvatar.tsx Lines 104 to 105 in 3ab4e6e
What changes do you think we should make in order to solve the problem?As for the <Tooltip text={translate('switcher.headerTitle')}>
<PressableWithFeedback
...
...
</Tooltip> And for the design aspect, as per comments from @shawnborton , we need to hover the button rather than the Icon, we can do so by implementing the following changes, the results are shown first:
simplescreenrecorder-2024-02-04_02.06.03.mp4The changes required are: First remove the existing Hover condition at {({hovered}) => (
<SubscriptAvatar
.
subscriptIcon={{
..
fill: theme.icon,
}}
.
hover={hovered}
/>
)} Then add hover as prop to And then set the style of the button to change on hover: {subscriptIcon && (
<View
style={[
.
.
hover && { backgroundColor: theme.buttonHoveredBG }
.
]}
.
> What alternative solutions did you explore? (Optional)N/A |
@thesahindia please review proposals when you have time 🙇 |
@dukenv0307's proposal looks good to me! 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
sorry I apparently clicked a wrong button and caused everyone to be unassigned 😓 @lakchote I'll keep assigning myself to this issue. I don't know whey Melvin assign you 🤷 |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
hey @thesahindia, according to comments from @shawnborton , he expected that the button BG be changed and not the icon, but @dukenv0307 proposal states to change the icon color:
And in my proposal i have followed the comments and updates the BG of the button rather than the icon, so isn’t my RCA more accurate? |
We generally handle these things in the PR. The issue is minor and the first proposal was good enough. |
@thesahindia The PR is ready for review. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.40-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-02-20. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
hi! Sorry I'm ooo sick today - I'll follow up on this tomorrow |
Payment summary Contributor: $250 @dukenv0307 (paid in Upwork - https://www.upwork.com/nx/wm/offer/28141432) Eligible for 50% #urgency bonus? NA Do we need a regression test for this? |
We don't need a specific test for this. |
awesome! thanks for confirming @thesahindia This is paid out based on this payment summary - #35618 (comment) Closing - feel free to reach out with any questions! |
$250 approved for @thesahindia |
Action Performed:
Expected Result:
There should be a tooltip that displays
Choose a workspace
.Only the background of the chevron icon should change, not the fill color of the chevron icon.
Please also read Shawn (our designer)'s additional comment here
Actual Result:
The tooltip doesn't appear.
The fill color of the chevron icon on hover changes on hover..
Workaround:
N/A
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Screen.Recording.2024-02-01.at.4.04.31.PM.mov
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: