Fix the back arrow on intial Load #8067
Conversation
|
@thienlnam This PR needs to go before awaiting PROD deploy so that it can be tested on next QA cycle with the main PR. |
|
@parasharrajat Good point, I'll prioritize this review then. Taking a look now |
There was a problem hiding this comment.
Changes look good, only have a small comment regarding comments and possibly a link
Testing steps need to be updated to be specific about which platforms need to be tested
Since you can only resize web and (maybe desktop?): It should indicate that we're checking resizing in those and in the rest of the platforms just verify that navigation around the app works
| // calculate the defaultStatus only once on mount to prevent breaking the navigation internal state. | ||
| // Directly passing the dynamically calculated defaultStatus to drawer Navigator breaks the internal state | ||
| // And prevents the Drawer actions from reaching to action Drawer Navigation while screen is resized on from Web to mobile Web. | ||
| defaultStatus: Navigation.getDefaultDrawerState(props.isSmallScreenWidth), |
There was a problem hiding this comment.
Small comments here:
// Calculate
Can you link to an issue regarding comment #2 if it exists?
// Prevents drawer action
There was a problem hiding this comment.
I will update it after your testing. But I don't have any links to the issue that I have mentioned here. Here is the explanation Why.
This component uses a key prop to remount the navigator when the browser resizes. This hack was done to prevent the layout from breaking on the drawer. Now due to that, it is somehow breaking the internal navigation state on change of defaultStatus. Rest of the explanation is in linked proposal.
|
Also ?? 😕 |
|
I don't know about this. Maybe, you have write access that's why you see this. the commit is actually from the forked repo and I am making a PR from fork to this repo. If you are seeing this first time, it is possible that Github added this feature recently. |
|
Hmm, it doesn't show up on your other commits from different PRs so I'm not sure what's happening. I'm noticing this is working off the merged branch, did you rebase master? (Maybe won't make a difference but mind merging in main after your next commit for comments?) |
|
Yeah, I used the old branch from the previous PR so maybe it is picking up that branch again as that was not deleted from newDot repo. it is up-to-date with main. |
|
Verified testing, just the small comments above left! |
|
Changes done. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @thienlnam in version: 1.1.42-0 🚀
|
|
🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀
|
Details
Follow-up PR of #7801. I reverted this part somehow from the previous PR before merging. This is the first part of the proposal #7091 (comment).
Fixed Issues
$ #7091
Tests
Contributor (PR Author) Checklist
main### Fixed Issuessection abovesrc/languages/*files (if applicable)Styling.md) for all style edits I madeSTYLE.md)Avatar, I verified the components usingAvatarare working as expected)mainbranch)PR Reviewer Checklist
main### Fixed Issuessection abovesrc/languages/*files (if applicable)STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)mainbranch)QA Steps
Note: Test this PR with #7801
Screenshots
Web
screen-2022-03-05_06.22.22.mp4
Mobile Web
screen-2022-03-06_13.34.11.mp4
Desktop
screen-2022-03-06_13.27.57.mp4
iOS
screen-2022-03-06_13.20.18.mp4
screen-2022-02-23_01.19.17.mp4
Android
screen-2022-03-06_12.01.23.mp4