-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix #3656: [RTL] High-fi Add support for RTL in Story Activity #3667
Conversation
Hi @veena14cs, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks! |
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.
Left one follow-up comment.
@veena14cs can you please also add tests for these changes to make sure they're well tested?
android:layout_height="wrap_content" | ||
android:layout_marginStart="8dp" | ||
android:layout_marginTop="8dp" | ||
android:layout_marginEnd="8dp" | ||
android:layout_marginBottom="4dp" | ||
android:fontFamily="sans-serif" | ||
android:textAlignment="viewStart" |
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.
Seems fine, though I'm unsure when we should be using textStart
vs. viewStart
. @veena14cs does textStart
have the same behavior? Per documentation it may be better to always default to textStart
since it's more likely to look correct (since it provides alignment based on the paragraph rather than the view), while also accounting for RTL scenarios.
Followed up with your comment and added tests for the same. |
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, thanks.
Thanks @rt4914 |
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 @veena14cs. This LGTM.
Unassigning @BenHenning since they have already approved the PR. |
Explanation
This PR fixes #3656 . This PR focuses on textalignment in
storyView
.Screenshots LTR and RTL
Mobile
....
.....
Tablet
......
.....
Checklist