[RNMobile] Scroll header toolbar to end when RTL #34617
Merged
Conversation
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
2 tasks
hypest
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +626 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
4 tasks
SiobhyB
approved these changes
Sep 7, 2021
Contributor
SiobhyB
left a comment
SiobhyB
left a comment
There was a problem hiding this comment.
LGTM 🚢 🚀
I tested this on an Android emulator and confirmed the changes fix the issue. 🎉 I also created an installable build for iOS (as I was having trouble seeing the RTL change on the iOS emulator) and confirmed there are no unwanted side effects on iOS when testing with a physical iPhone XR.
I'm curious why this worked as expected on iOS both with and without this change. Is there a known reason I'm missing or perhaps a quirk with the way ScrollView works? Either way, the change looks good to my eyes and I've approved it. :D
Contributor
Author
|
Thanks for the review @SiobhyB and for preparing an installable build of WPiOS!
Good point! When I tested using a physical iPhone against Metro it looked indeed that the changes here work, but your note now prompted me to check deeper. Do you mind if I also update your test WPiOS PR alongside? |
Contributor
Ah, really interesting! Thank you for digging deeper and sharing that. I'll read through and bear in mind for the future.
Not at all, feel welcome to update the PR in any way. |
Contributor
Author
|
👋 @SiobhyB ! Ready for another review! I updated all the branches, included the commit to only apply the fix to Android and have triggered an installable build for iOS. Thanks! |
SiobhyB
approved these changes
Sep 10, 2021
Contributor
SiobhyB
left a comment
SiobhyB
left a comment
There was a problem hiding this comment.
LGTM! I tested again on the Android demo app and with the iOS installable build. All worked smoothly in my testing 🎉 🚀
I left a question and one suggestion for the CHANGELOG, approving this PR as neither of those are blockers and this looks good!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes the initial horizontal positioning of the main header toolbar when RTL.
Related PRs
How has this been tested?
Using the gb-mobile PR
Screenshots
toolbar-scrolling-to-inserter-icon-when-rtl.mp4
Types of changes
HeaderToolbaris executing ascrollToStartfunction when the content size changes, attempting to always make sure the inserter button gets visible. This PR updates thescrollToStartto respect the RTL setting.Checklist:
*.native.jsfiles for terms that need renaming or removal).