-
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-07] [$500] [Wave 8 - Ideal Nav Skeleton] Update the placement of the user status icon #34380
Comments
Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new. |
Job added to Upwork: https://www.upwork.com/jobs/~01f723a88aa92b6d67 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
This comment was marked as outdated.
This comment was marked as outdated.
@gijoe0295 Please be descriptive about the solution and refrain from adding vague speed run Proposals. |
@ishpaul777 Sure. |
Just my opinion, this might make difficult to navigate to status directly as size is too small on touch screen, and may somtimes result in unexpected navigation to stauspage when use expect to go to settings. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Update the placement of the user status icon What is the root cause of that problem?this is a new feature What changes do you think we should make in order to solve the problem?We are using AvatarWithOptionalStatus component for showing the LHN avatar with status emoji, here we are using the parent component that has width of 84, we should update styling here, and width will be 44, same as height of the Avatar in this case, Line 3915 in 46f011f
in above styling we only need height, width and margin other styling can be removed we have to place the emoji to the bottom so, we have adjust 2 views
PressableAvatarWithIndicator will be on top and beneath we will add the emoji View, so View hierarchy will be like this
next we have to update the styling for the status parent View in this case, sidebarStatusAvatar styles, we can directly apply style to the View or we can apply it to the main PressableWithoutFeedback, I have tested with parent View for the styling we can pass the height/width 22 (as subscript is half of the main view), but here we are passing the fontsize 22 to the emoji Text,so we can adjust style accordingly, I have used below style to achieve the desired outcome in the Results
What alternative solutions did you explore? (Optional)none |
The proposal from @jayeshmangwani looks good to me! We still need to ask the design team for the correct size. 🎀 👀 🎀 C+ reviewed! |
Triggered auto assignment to @johnmlee101, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Yeah, I don't think you can navigate to the status directly anymore in the new design -- it's an incredibly small press area. We talked about using like a popover menu for things like status, switching accounts etc but that didn't materialise. So I think there's only one press action on the account avatar that navigates to the account setting page going forward. CC: @shawnborton @dannymcclain @dubielzyk-expensify |
We could increase the hitSlop area of the status button just a tad, so you gain a little more tappable space. Otherwise yeah, we're going to have to live with this one until we implement something a bit more drastic as Tom is describing above. But I actually still think we should keep the two separate link paths - the rest of the avatar goes to your profile, and the status button - even as small as it is - still goes to the status page. |
Fine by me! 👍 |
I'm okay with the direction Shawn mentions, but my gut says that it'll be too hard to distinguish the taps especially on mobile. Let's go with it though and adjust from there |
Sweet, let's try it! Agree, mobile might be tricky to hit the correct target but at least desktop users will be able to take advantage of it. |
Nice! @johnmlee101, can you give a secondary of this proposal please? |
LGTM! 👍 |
📣 @mollfpr 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @jayeshmangwani 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.33-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-07. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Regression test
@trjExpensify Could you give the payment summary for manual request? Thank you! |
Yo yo! Thanks! I'm going to hold off on that regression test, as it'll get encapsulated into the changes for ideal nav 👍 Payment summary as follows:
|
@jayeshmangwani - paid! Closing! |
$500 approved for @mollfpr based on summary above. |
👋 Coming from here.
With the new ideal nav layout the status indicator will end up looking like this:
So we want to update it to this:
I don't see why we can't go ahead and have this be worked on now, but let me know if anyone thinks otherwise.
CC: @hayata-suenaga @mountiny @JmillsExpensify @kosmydel @adamgrzybowski
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: