-
Notifications
You must be signed in to change notification settings - Fork 15k
Add RTL support #51600
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
Add RTL support #51600
Conversation
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Relevant to PR #51598 |
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 for working on this.
Can you describe the testing you've done, especially the work to test that this doesn't break existing content? The more we - reviewers - can see that it's already been tested, the easier it is to review.
It's OK to use tools to check that existing pages still render like they did before.
assets/scss/_desktop.scss
Outdated
@@ -3,6 +3,19 @@ $vendor-strip-height: 44px; | |||
|
|||
@media screen and (min-width: 1024px) { | |||
|
|||
body:lang(fa) { |
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.
Why just Persian / can we use a different selector that is reusable across RTL locales?
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.
Yes, Fixed
assets/scss/_documentation.scss
Outdated
.alert:dir(rtl) { | ||
border-style: solid; | ||
border-width: 0 4px 0 0; | ||
border-radius: 0 0 0 0; |
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.
nit: this set of radii doesn't look correct
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.
assets/scss/_documentation.scss
Outdated
border-style: solid; | ||
border-width: 0 4px 0 0; | ||
border-radius: 0 0 0 0; | ||
h4 { |
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.
no need to override this from bold to bold
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.
Is the above comment sufficient for the issue you raised?(#51600 (comment))
assets/scss/_tablet.scss
Outdated
@@ -18,6 +18,15 @@ $vendor-strip-font-size: 16px; | |||
display: none; | |||
} | |||
|
|||
body:lang(fa) { |
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.
Why just Persian / can we use a different selector that is reusable across RTL locales?
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.
Yes, Fixed
f3fd3bd
to
a90cf9e
Compare
Since it turns out that the changes only affect RTL pages, there is no need to worry about pages that were working properly until now. but I did some test that I'm going to share it below: on
After that I ran on
After that, I used the
In the end, I used
The only changes in all these files are two things: the addition of I performed the tests with these tools, but if you're aware of a better or faster option, let me know. |
/area localization |
Can anyone from the nascent Persian team review this? (you can write a review so long as you have a GitHub account and you promise to follow our code of conduct) |
No approval is needed, but we do want feedback from people who routinely work in an RTL locale. |
I'm involved in the effort to localize the Kubernetes website into Persian and have checked the changes introduced here. |
assets/scss/_documentation.scss
Outdated
.alert:dir(rtl) { | ||
border-style: solid; | ||
border-width: 0 4px 0 0; | ||
border-radius: 0 0 0 0; |
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.
border-radius: 0 0 0 0; |
Try it without this change to border radius; see what you think. You should leave the change to border-width
which is correct.
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.
also done
assets/scss/_documentation.scss
Outdated
h4 { | ||
font-weight: bold; | ||
font-style: initial; | ||
} | ||
} |
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.
h4 { | |
font-weight: bold; | |
font-style: initial; | |
} | |
} |
Only override things if they are relevant to the configurable writing direction.
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.
right, done
assets/scss/_tablet.scss
Outdated
table:dir(rtl) { | ||
text-align: left; | ||
direction: ltr; | ||
} |
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.
Why this change, covering all tables? It at the very least needs an explanatory comment.
Ideally, we make the table shortcode support configurable table direction. If we can't or won't do that, let's discuss why.
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.
Because all tables in Persian must be from right to left, exactly the opposite of English. Therefore, it must be applied to all tables.
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.
Similar to the previous comment, text should be right aligned.
@@ -118,6 +127,13 @@ $vendor-strip-font-size: 16px; | |||
text-align: left; | |||
margin: 0 5% 15px 0; | |||
} | |||
|
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.
Optionally:
&:dir(ltr) > .light-text { | |
display: block; | |
float: right; | |
text-align: left; | |
margin: 0 5% 15px 0; | |
} | |
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.
I think only override RTL ones and do not change anything related to current state of Kubernetes website
assets/scss/_tablet.scss
Outdated
&:dir(rtl) > .light-text { | ||
display: block; | ||
float: right; | ||
text-align: right; | ||
margin-right: 5%; | ||
} |
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.
Why do this? It seems odd.
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.
@@ -1,5 +1,5 @@ | |||
<!doctype html> | |||
<html lang="{{ .Site.Language.Lang }}" class="{{.Params.class}} no-js"> | |||
<html lang="{{ .Site.Language.Lang }}" class="{{.Params.class}} no-js" dir="{{ or .Site.Language.LanguageDirection `ltr` }}"> |
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 just right.
assets/scss/_desktop.scss
Outdated
table:dir(rtl) { | ||
text-align: left; | ||
direction: ltr; | ||
} |
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.
Why this change, covering all tables? It at the very least needs an explanatory comment.
Ideally, we make the table shortcode support configurable table direction. If we can't or won't do that, let's discuss why.
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.
Because all tables in Persian must be from right to left, exactly the opposite of English. Therefore, it must be applied to all tables.
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.
So, for content that is right to left, this forces the text direction to be ltr
?
It seems odd and I don't understand the intent here.
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 LTR languages, we lay tables out LTR and left-align the text by default. I think that applies to all our localizations (modulo that some can also be written vertically, eg Japanese).
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.
I agree with Tim, that the tables in RTL, should have RTL direction and text should be right aligned.
a90cf9e
to
3ea4fe6
Compare
BTW, if you want to demo this:
[If you do that, we can preview how tables will look in RTL.] |
3ea4fe6
to
3ec8c24
Compare
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, but someone who reads a right-to-left locale should give this a final once over.
} | ||
|
||
.breadcrumb-item + .breadcrumb-item::before { | ||
float: right; |
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.
Doesn't this affect breadcrumb of ltr pages?
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.
Yes, you’re absolutely right, and I’ve fixed it.
Many thanks, @bsalamat
3ec8c24
to
485b0a5
Compare
/lgtm |
LGTM label has been added. Git tree hash: 3cbd3d7ac62cbc0f3f18490bbffba072843a3f97
|
/lgtm Thank you very much, @xirehat! I really appreciate your work to make Kubernetes universally accessible. |
If we're wrong about the impact, we can revert this. /approve @xirehat—thank you very much for shepherding this through to approval. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lmktfy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add RTL support to the Kubernetes website.