Skip to content
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 #4886

Merged
merged 5 commits into from
Jun 25, 2024
Merged

Add RTL Support #4886

merged 5 commits into from
Jun 25, 2024

Conversation

yaim
Copy link
Contributor

@yaim yaim commented Jun 21, 2024

This is an enhancement or feature.

Summary

As you might know, some writing systems are written and read from right to left (mainly Persian, Arabic & Hebrew). These types of writings need extra formatting for web.

In this PR I added the support for RTL direction with the minimum changes in the code by only adding an extra css file, & not changing the html structures and leaving zero side effect for the original left to right view.

These changes are just meant to be proof of concept and are not final changes. I was not sure if RTL support is a concern for the project or not, but if it is, then I'll be more than happy to make them ready for a final review.

Context

RTL support was previously mentioned in #3903 but the suggested changes where affecting the original LTR direction.

Live Demo

Here's a live demo of the RTL support.

@yaim
Copy link
Contributor Author

yaim commented Jun 21, 2024

@ha36d I noticed you have submitted a related PR (#4884), 15 minutes before me. 😅
I'll be glad if we can work on it together.

@ha36d
Copy link
Contributor

ha36d commented Jun 21, 2024

@ha36d I noticed you have submitted a related PR (#4884), 15 minutes before me. 😅 I'll be glad if we can work on it together.

Yes, Sure. I was moving from blogger to jekyll. So I needed RTL support on this theme.

@yaim
Copy link
Contributor Author

yaim commented Jun 21, 2024

Awesome!
The changes I suggested are activated by adding a

direction: ltr

to the site settings sections in _config.yml.

Here's the config for the demo link mentioned in the PR description.

@ha36d
Copy link
Contributor

ha36d commented Jun 21, 2024

Awesome! The changes I suggested are activated by adding a

direction: ltr

to the site settings sections in _config.yml.

Here's the config for the demo link mentioned in the PR description.

For me, it is the same. But I am not introducing css changes. It only makes the body page "RTL". I guess still we need the body direction in "rtl".

@yaim
Copy link
Contributor Author

yaim commented Jun 21, 2024

Correct. I added the body direction to CSS, but I think adding the dir attribute is more appropriate.
But the rest of the CSS extensions were needed to keep to meaning of the template.

@ha36d
Copy link
Contributor

ha36d commented Jun 21, 2024

Correct. I added the body direction to CSS, but I think adding the dir attribute is more appropriate. But the rest of the CSS extensions were needed to keep to meaning of the template.

Agreed. you can copy the documentation part from mine. I will close my PR.

@yaim
Copy link
Contributor Author

yaim commented Jun 21, 2024

Thanks! Cherry picked your doc change! 🙌🏻

@ha36d ha36d mentioned this pull request Jun 21, 2024
@iBug
Copy link
Collaborator

iBug commented Jun 23, 2024

Looks decent. I have one minor concern: For the configuration key, why did you choose direction: rtl instead of a boolean key like rtl: true? To me it seems easier to maintain and you don't have to mention the default value (which is actually null).

@yaim
Copy link
Contributor Author

yaim commented Jun 24, 2024

Thanks for the feedback @iBug, that totally makes sense!
I also noticed the toc does not expand in mobile view which is not the as original version. I'll fix both soon and ask for your review.

Also I thought maybe it's better to inject a rtl value into body's attributes, instead of loading a whole new file and then have the rtl condition styles next to the parts their original style is defined. I think that would be easier to maintain and if someone decided to change the style, they would also notice there's a rtl counterpart. What do you think?

@yaim
Copy link
Contributor Author

yaim commented Jun 24, 2024

Aaaaah, there's actually an exact example in the scss docs:

  // It can also be used to style the outer selector in a certain context, such
  // as a body set to use a right-to-left language.
  [dir=rtl] & {
    margin-left: 0;
    margin-right: 10px;
  }

@yaim
Copy link
Contributor Author

yaim commented Jun 25, 2024

@iBug I pushed the change you mentioned + fix for the bug I saw on small screen view.

Also made the removing separate rtl file suggestion in the 3rd commit (9ebd124).
If you think it's OK, I'll fixup it to the first commit, but if you don't like that, I can drop it.

Thanks! 🙌🏻

_layouts/default.html Outdated Show resolved Hide resolved
@iBug iBug merged commit 621529a into mmistakes:master Jun 25, 2024
1 check passed
@yaim yaim deleted the feature/add-rtl-support branch June 25, 2024 11:55
@iBug
Copy link
Collaborator

iBug commented Jun 25, 2024

Seems sufficiently good for now so I've merged it. If you have any further improvements feel free to open another PR.

One potential improvement I can think of is replacing all affected margins and paddings with padding-block-start and similar, so that you don't have to, for example, set *-left: 0 while *-right: (original value). Browser support for these direction-agnostic CSS properties are not to be worried.

@iBug
Copy link
Collaborator

iBug commented Jun 25, 2024

I've pushed a few extra commits and I wonder if you'd like to help test it (e.g. remote_theme: "mmistakes/minimal-mistakes@master").

@yaim
Copy link
Contributor Author

yaim commented Jun 26, 2024

Sorry for late reply.
All my test cases look valid on the changes! They look great! 🙌🏻

@srezasm
Copy link

srezasm commented Jul 5, 2024

Hi, thanks for your work. It would be great to dynamically adjust the direction of the paragraph based on the used language.

Just like the way that GitHub handles RTL languages in rendered markdown views:

همانطور که مشاهده می‌کنید، جهت این پاراگراف از راست به چپ است.

@iBug
Copy link
Collaborator

iBug commented Jul 5, 2024

@srezasm If you use Kramdown's syntax for adding HTML attributes, you'll automatically have RTL for only specified paragraphs:

This is some text.

This is some more text.
{: dir="rtl" }

@srezasm
Copy link

srezasm commented Jul 5, 2024

@iBug Can it be automated by the language that paragraph is started with?

@iBug
Copy link
Collaborator

iBug commented Jul 5, 2024

@srezasm Not sure. Maybe you can go with dir="auto" and hope your browser does what you want.

@srezasm
Copy link

srezasm commented Jul 5, 2024

@iBug It works with dir="auto" as well, but still, I will need to add this at the end of each paragraph manually.

@iBug
Copy link
Collaborator

iBug commented Jul 5, 2024

@srezasm You can probably just add one dir="auto" to the <body> element as this attribute can be inherited.

@srezasm
Copy link

srezasm commented Jul 5, 2024

@iBug Didn't work, do you know how to add it to all individual text placeholder tags?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants