-
-
Notifications
You must be signed in to change notification settings - Fork 5
Remove unnecessary horizontal scroll in RTL UI #3263
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3263 +/- ##
=======================================
Coverage 82.55% 82.55%
=======================================
Files 605 605
Lines 35313 35313
Branches 5745 5740 -5
=======================================
Hits 29152 29152
+ Misses 5313 5301 -12
- Partials 848 860 +12 ☔ View full report in Codecov by Sentry. |
Nateowami
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 is kind of a blunt-force approach that doesn't get to the root of the problem, and could lead to different problems in the future. Based on my investigation, I believe the problem stems from .cdk-live-announcer-element using left: 0 instead of the logical inset-inline-start: 0
Reviewable status: 0 of 1 files reviewed, all discussions resolved
2e038d4 to
49a7b35
Compare
RaymondLuong3
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.
Ok, I found that it was a combination of the .cdk-live-announcer-element and the .cdk-describedby-message-container that had the cdk-visually-hidden class which had the left: 0 style defined. The style is defined in a _index.css file that I believe comes from angular. The style does have a definition for the containing element having dir='rtl' defined, but in this case it wasn't working. I had to manually add that rule to our styles file.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
|
Why not just use a logical property, so nothing has to change between ltr and rtl? |
49a7b35 to
8020a08
Compare
RaymondLuong3
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. I introduced the inset-inline-start rule which does work. The left: 0 rule is therefore valid when the UI is in RTL which is unnecessary, but it appears it doesn't affect the layout negatively.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
8020a08 to
f8258b1
Compare
pmachapman
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)
A horizontal scroll bar appears when the UI is changed to RTL. I think this occurred when we updated to implement material theme, but I did not go back to confirm. This change removes the unnecessary horizontal scroll bar.
This change is