[Desktop Navigation] Drop the <Workspace> from the LHN headers#59575
Merged
mountiny merged 11 commits intoExpensify:mainfrom Apr 11, 2025
Merged
Conversation
Contributor
Author
|
@Expensify/design cc: @mountiny |
Contributor
|
Just triggered the app build for this one! |
Contributor
|
Sorry if this is a silly question, but how do I get added to the beta for this?
|
Contributor
In Supportal - I just added you! In other news it seems like the app build failed, not sure why: 
Maybe someone else want to give it a try? It's the first time I did it so maybe I missed something? |
Contributor
|
🚧 @dannymcclain has triggered a test app build. You can view the workflow run here. |
Contributor
|
Just tried running the build again, from that auto-comment above looks like it might be happening?? |
This comment has been minimized.
This comment has been minimized.
Contributor
|
🤔 Not sure why the app builds are failing. Also cc @Expensify/design to take a look! Looking pretty good so far to me (on the web build at least). |
Contributor
|
I've been seeing mobile builds fail all over the place - we might need to pull in main into this branch and see if that fixes it. Will check out desktop though! I just added myself and @dubielzyk-expensify to the beta, it might take a while to be enabled though. |
Contributor
|
Okay nice, just did a clear cache and restart and that gave it to me. Feeling good on my end too! |
Contributor
|
Oh it's looking great 😍 Can't wait to see the LHB! |
Contributor
|
@WojtekBoman can you sync with main then the builds should work |
Contributor
|
(Thanks for adding me to the beta @shawnborton ) Tested the web build and it looks pretty good to me too. |
Contributor
|
Actually, checking mobile web and it looks like the search icon has too much padding on the right:
You can see in the Workspace Editor it's different: I believe the spacing here should be 12px to accomodate for the whitespace of the icon, but @Expensify/design can correct me: |
Contributor
|
Ah good catches. Alternatively, we remove the 40x40 bounding box, and just show the 20x20 icon over to the right. And then we can add some extra |
WojtekBoman
force-pushed
the
improve-desktop-nav/remove-workspace-from-headers
branch
from
5b50d2e to
58b11a4
Compare
Contributor
Author
|
@Expensify/design I updated it this way: |
Contributor
|
@WojtekBoman Does it have enough hitSlop around the icon now though so that the user on native/ touch device can also click outside of it slightly to trigger the click? |
Contributor
|
Yeah, visually that looks good but I agree we should add some hitSlop to extend the touch area. |
Contributor
Author
|
I'll add extra 12px hitSlop from the left and right of the icon EDIT: I added hitSlop using prop |
Contributor
|
Great catch Jon! And I agree—visually the update looks nice, let's just make sure we've got a big enough hit area for each icon. I think it needs to be at least 40x40px. |
Contributor
Author
|
Could you trigger new builds and check the latest changes? cc: @flaviadefaria @mountiny @Expensify/design |
Contributor
|
🚧 @dannymcclain has triggered a test app build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Contributor
|
That looks better to me. While we're at it... maybe we can kill the unnecessary 8px horizontal padding from the Cancel button when you focus into the router from Reports? |
Contributor
|
Icon positioning is looking good for search on mobile, but I can't really tell what the hit area is. I also am not seeing the help icon in my account so I can't verify that one 😕 |
Contributor
Author
Done! |
Contributor
|
Contributor
|
Looks better now |
Contributor
|
Nice, let's get this into final review. Seems pretty good at first glance 👍 but let's see if the other designers have feedback too. |
Contributor
|
Feeling pretty good to me! |
Contributor
|
Great, thank you! All yours @dukenv0307 |
dukenv0307
approved these changes
Apr 10, 2025
Contributor
|
We're good to merge @mountiny |
Contributor
|
Builds are looking good for me as well 👍 |
mountiny
approved these changes
Apr 11, 2025
Contributor
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Very exciting, looks good to me, lets go
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
Bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊 (1/10)Significant Changes To Duration
Show details
|
Contributor
Performance Comparison Report 📊 (10/10)Meaningless Changes To Duration (9/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (2/10)Meaningless Changes To Duration (1/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (3/10)Meaningless Changes To Duration (2/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (4/10)Meaningless Changes To Duration (3/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (5/10)Meaningless Changes To Duration (4/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (6/10)Meaningless Changes To Duration (5/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (7/10)Meaningless Changes To Duration (6/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (8/10)Meaningless Changes To Duration (7/9)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (9/10)Meaningless Changes To Duration (8/9)Show entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
mountiny
removed
the
DeployBlockerCash
Contributor
|
This seems as incorrectly flagged. The PR is mainly changing styles so cant see how it could slow down App start TTI |
Contributor
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.28-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.28-15 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This PR introduces following changes:
To see these changes it's necessary to have an account with access to the
leftHandBarbetaFixed Issues
$ #59369
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop