-
Notifications
You must be signed in to change notification settings - Fork 3.5k
NavigationTabBar: Add dummy component #63242
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
NavigationTabBar: Add dummy component #63242
Conversation
Problem
Those components are some of the most commonly used in the app, making Currently the cost of mounting
SolutionImplement Potential cost of mounting
|
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks promising, @adamgrzybowski @WojtekBoman any concerns with such approach as at least some sort of improvement?
| return <NavigationTabBarNarrowDummy selectedTab={selectedTab} />; | ||
| } | ||
|
|
||
| return <NavigationTabBarWideDummy selectedTab={selectedTab} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case do we need this in the Wide layout? Could we not render this at all in wide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! It is there to make dummy counterpart 1:1 match with regular component, but we can investigate it further to see how it behaves on wider screens 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to see the navigation bar under the overlay (the one from RHP), we need to render it per screen on the wide layout. The top level is displayed over everything else. It wouldn't be covered by the overlay.
|
@ishpaul777 will you be able to review? |
|
yes i can review @mountiny |
|
@ishpaul777 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] |
| <EducationalTooltip | ||
| shouldRender={false} | ||
| anchorAlignment={{ | ||
| horizontal: isWebOrDesktop ? CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER : CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, | ||
| vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM, | ||
| }} | ||
| shiftHorizontal={isWebOrDesktop ? 0 : variables.navigationTabBarInboxTooltipShiftHorizontal} | ||
| renderTooltipContent={noop} | ||
| wrapperStyle={styles.productTrainingTooltipWrapper} | ||
| shouldHideOnNavigate={false} | ||
| onTooltipPress={noop} | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltips are displayed only on the top level so we don't need them for the dummy componenent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, thanks! ✅
|
I don't know if it's a problem, but I noticed one UI change Screen.Recording.2025-06-02.at.15.29.12.mov |
b822093 to
13a055a
Compare
Thanks for spotting this @WojtekBoman! It is unfortunately a tradeoff of not including responsible logic in order to make dummy component as simple as it gets - we'd need to pretty much go back to what full blown navigator consist of - ordered reports, report attributes etc. With that said, let me know what you think @mountiny @ishpaul777 tl;dr; this part of the UI consistency requires heavy logic to be plugged in |
|
As @mountiny suggested, I am thinking whether we need those duplicates on wide screen at all - @adamgrzybowski do you know if wide screen UI also suffers from transitions? If not, we might skip the rendering at all in wide screen scenarios. |
|
Its its mainly for he overlay it feels like we do not need it. However, I think the bug @WojtekBoman found is definitely a blocker for this PR as is. We do need to keep the RBR/ GBR, its bad UX if it would be hiding and re-appearing |
|
Agree @mountiny 👍 In this case let's pivot the plan and:
I will check what are the possibilities and new outcomes. Let me know if new plan sounds good :) |
|
I still struggle to remember why we would need it in Wide view but also we used to have the tabs in the bottom and now they are on the Left in wide view so it might have been something specific to the previous UI. Definitely up for trying to get rid of the extra navigator in wide layout |
On a wide layout we use TopLevelNavigationTabBar to display tooltips and avoid flickering when switching tabs (when NavigationTabBar is re-rendered in each tab, RBR/GBR may flicker) |
@WojtekBoman Sorry my knowledge is limited in this case, but I would like to understand it fully. Wouldn't singular Also, it's rendered once on top and stays there and I am not sure what flickering we could experience. The behaviour should also remain the same as in duplicated Could you show what is the regression when using top level navigator only? Thanks ❤️ |
|
I updated my answer, I wanted to emphasize why we need TopLevelNavigationTabBar
Yes When I mentioned flickering, I meant that with TopLevelNavigationTabBar we avoid flickering because we don't unmount the bar when switching tabs |
We use navigation tab bars on each sidebar screen to display them below RHP. If we rely only on TopLevelNavigationTabBar, it will be displayed above the RHP overlay Screen.Recording.2025-06-03.at.09.55.47.mov |
|
I've chedk and it's indeed hard to work around current stacking context (layering) flow on web to display TopLevle without issues when RHP is open. In this case, I can limit current efforts to:
Sounds good @mountiny? Based on the results we can use it or not if the improvement is negligible. |
|
@kacper-mikolajczak sounds good, if we can use it there at least that would be handy |
|
If the bottom tabs would have gbr or RBR though, then you slide to close the page using a gesture, the dummy component would not have them and it could cause a flicker, right? |
We will keep all the logic required not to undermine functionality in any way. In practice, mostly FAB is going to be trimmed away. I am curious what's the perf diff in this case, will report you back 🔜 |
|
Result of applying RBR indicators logic on web (commit) shows no particular difference in render timings - we should be good to go! web-after-rbr.mp4 |
|
@kacper-mikolajczak what about the GBR? |
|
@ishpaul777 can you proceed with the review and testing please? |
|
@kacper-mikolajczak prettier |
|
catching up! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-06-05.at.12.29.45.AM.movAndroid: mWeb ChromeScreen.Recording.2025-06-05.at.12.38.28.AM.moviOS: HybridAppScreen.Recording.2025-06-05.at.12.06.41.AM.moviOS: mWeb SafariScreen.Recording.2025-06-05.at.12.02.04.AM.movMacOS: Chrome / SafariScreen.Recording.2025-06-04.at.11.46.19.PM.movMacOS: Desktop |
ishpaul777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, tests well! please fix prettier
|
@ishpaul777 Prettier has been fixed ✅ |
ishpaul777
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
| return <NavigationTabBarWideDummy selectedTab={selectedTab} />; | ||
| } | ||
|
|
||
| export default memo(NavigationTabBarDummy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Display name
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Found an issue while testing #62747, might be related to this PR:
Expected result: Navigation bar works Screen.Recording.2025-06-09.at.22.37.20.mov |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.63-0 🚀
|
|
@kacper-mikolajczak can you please take a look at reported bug here |
|
Thanks for raising! |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.1.63-6 🚀
|
Explanation of Change
Fixed Issues
$ #63244
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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))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