mmistakes / minimal-mistakes

:triangular_ruler: Jekyll theme for building a personal site, blog, project documentation, or portfolio.
https://mmistakes.github.io/minimal-mistakes/
MIT License
12.05k stars 24.98k forks source link

Add RTL Support #4886

Closed yaim closed 1 week ago

yaim commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Thanks! Cherry picked your doc change! 🙌🏻

iBug commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

@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 (9ebd124ad86bccdad3992bfd017855332f986eb2). 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! 🙌🏻

iBug commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

srezasm commented 2 days ago

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 commented 2 days ago

@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 commented 2 days ago

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

iBug commented 2 days ago

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

srezasm commented 2 days ago

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

iBug commented 2 days ago

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

srezasm commented 2 days ago

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