Skip to content

Add RTL support#30980

Merged
XhmikosR merged 10 commits into
mainfrom
master-fod-rtlcss
Dec 4, 2020
Merged

Add RTL support#30980
XhmikosR merged 10 commits into
mainfrom
master-fod-rtlcss

Conversation

@ffoodd

@ffoodd ffoodd commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

Fixes #30918, fixes #24807, fixes #27783, fixes #31845

PreviewExamplescheatsheetRTL cheatsheet

Todo

Beta 2

  • Move cheatsheet from examples to main docs
  • Allow main docs to be passed a direction in the front matter and have that automatically toggle LTR to RTL CSS inclusion
  • The above items should resolve most of this, but... De-dupe the RTL CSS once moved
  • Convert page content to Markdown
  • And Add RTL support #30980 (comment)

Cheatsheet

  • use buttons in cheatsheet side navigation, as it's been done in our main docs recently
  • Fix the sticky components title covering the page content
  • Fix floating labels example (remove row)
  • Make the header a little less huge
  • check and update translations in examples and cheatsheet

Docs

Done

  • add RTLCSS and dedicated scripts
Test cases
Spotted bugs
Documentation
Chore things
  • add files to SRI hashes and docs' _config.yml
  • RTL: refactor using logical names #31210 — consider renaming things, ala Logical properties—to prevent .float-left { float: right; }. It would also help to switch to logical properties one day (when dropping Legacy Edge, v6 or tomorrow with Revisit our browserslist config #30986 ?)
  • add RTL files to Bundlewatch
  • double check the css-rtl task: something feels weird when it comes to start the docs, since it builds the dist then post-processes it for RTL.
  • Missing a way to correctly handle modal's padding in two ways. I fixed a case but I guess there's more…
    • and it will obviously require new test, I guess?
  • proof-read JS changes: I added a utility to check if document is using dir="rtl" and calling it where appropriate to toggle right / left.
  • double-check package.json changes:
    • I duplicated css-minify to prevent a hundreds cols long script, but it's basically the same, split on two parts (standards + RTL)
    • I added a watcher for dist/css/ to reload when RTL files are compiled again (since they're not compiled from our sources but from our dist files)
  • fix sourcemaps for RTL dist files
    • FWIW, sourcemaps are fine: there's a single path ending URL encoded, but it's not part of our sources: %3Cinput%20css%202%3E
    • I think it relates to the /*rtl:raw*/ directive, used for [type="email"] for example;
    • Other /* rtl:* */ directives are correctly mapped to their sources.
    • Opened an issue upstream to have some better understanding.
  • (optional) run examples CSS through RTLCSS (dashboard, carousel, blog) to use a single source for both LTR & RTL
    • @XhmikosR I managed to make it work as intended, however I'm stuck with setting the full output path since glob patterns don't seem to work here (so we have /5.0/) in --dir and --base).
  • check new examples' screenshot (old logo?)
  • make a new dist PR separately before we land this one (Dist #32216)

Questions

  • About the new Cheatsheet example: should we add introduction text before each component or section? → Probably not, since it's mostly a detailed view not meant to replace docs.
  • I used RTLCSS block-level syntax for directives, but RTLCSS' fork used property-level syntax. Block-level is lighter to output and faster to parse, but make it less obvious of what is important to ignore for RTL. Should I move to property-level syntax? → Switched to property-level syntax, as argued in comments.

References

Related

RTL concerns

#28797 #28238 #24807 #27123 #27122 #26879 #26818 #19545 #26299 #25422 #24662 #23703 #24332 #23117 #22708 #22137 #21619 #21180 #20293 #19555 #20075 #19787 #19703 #19668 #19643 #19545 #19379 #18184 #17595 #16455 #16419 #15717 #15700 #15509 #15479 #15433 #14717 #14510 #13564

Kitchen sink of sorts

#18645 #27783 #21209 #17423

@ffoodd

ffoodd commented Jun 9, 2020

Copy link
Copy Markdown
Contributor Author

@twbs/team Content strategy concerns here, before going further: how/where should we show and test RTL?

As a simple basis, I just duplicated our firt example, "Album" using a new examples-rtl.html layout and a condition to use bootstrap.rtl.min.css if the layout is used.

So, two questions:

  1. should I keep going with those duplicated examples?
  2. what about testing our components? How do we ensure future changes does not harm RTL version?
    1. do we provide some kind of sandbox or kitchen sink with every components in RTL?
    2. do we try to display RTL version alongside our current docs (with, say, something like an iframe with srcdoc—which could be handled via a shortcode)
    3. maybe need an entire new section for our docs?

"Duplicated" examples looks fine IMHO, since they also allow to translate content (which would be very valuable). But when it comes to component, I have no strong opinion ATM.

FYI, Boosted used a very ugly script to automatically duplicate examples and a sort of Kitchen Sink named "Boostwatch"—also available in RTL. None of those seems fine to me.

@mdo

mdo commented Jun 10, 2020

Copy link
Copy Markdown
Member

Regarding content strategy, I think we need at 2-3 pieces:

  • Docs page (perhaps under Getting Started or Customize) to document how we build our RTL dist files. This should include how to build with these files (which is likely a small modification from the Customize docs).

  • I don't think we need to do all our examples, but up to three sounds reasonable if we think it makes sense. One at the minimum.

  • For testing components, I think this is where we need to create a kitchen sink of sorts—all our components and their most common (or least common lol) variations in one page. This then could be duplicated and shown with RTL enabled.

    This kitchen sink page has for sure been requested and is something I know the community would appreciate. Putting the right love into it could be something folks could rally around for customizing Bootstrap for their own needs, and extending it as needed.

    I think this page could live as an example, or as a top level page like Migration (docs/5.0/cheatsheet/).

@ffoodd ffoodd force-pushed the master-fod-rtlcss branch 3 times, most recently from 7a0569b to 3c30ab7 Compare June 11, 2020 13:52
@ffoodd

ffoodd commented Jun 12, 2020

Copy link
Copy Markdown
Contributor Author

I tried to automate the Cheasheet page by parsing our components' markdown files, finding examples via a regex (ala scss-docs shortcode) and displaying them. It somehow works, but seems like a dead end to me since our examples are done in multiple ways (including datas, loops, etc.), and some of them require either a few JavaScript or custom styles to work well, which I doubt being able to get easily.

So I'll go for a dedicated example, hand-managed for now: if anyone has any idea to improve this, feel free to mention or PR against this branch :)

Edit: For now, I manually wrap up component examples, trying to be both exhaustive and concise. I'm still missing Content, Forms and Cards.

@ffoodd ffoodd force-pushed the master-fod-rtlcss branch from 8aa0c74 to f6c048c Compare June 12, 2020 13:39
@mdo

mdo commented Jun 12, 2020

Copy link
Copy Markdown
Member

This is so encouraging to see! <3

@ffoodd

ffoodd commented Jun 13, 2020

Copy link
Copy Markdown
Contributor Author

Note to myself:

  • there's probably a way to use frontmatter for RTL layouts, instead of duplicate layout

@mdo mdo mentioned this pull request Jun 16, 2020
@mdo mdo changed the base branch from master to main June 16, 2020 19:32
@ahmadalfy

Copy link
Copy Markdown

Hello, I've extensive experience working with RTL and I've been using logical properties and values in production for almost four years with fallbacks. I wrote a couple of guides for RTL one of them is published on CSS Tricks and I would love to help. Any suggestions where can I start looking?

@ffoodd

ffoodd commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

Hi @ahmadalfy, very glad to have some help!

At the moment, you may help by checking the five example templates I converted to RTL:

For now, I only ran out CSS through RTLCSS, and translated roughly their content to arabic (with tools).

So I guess there can be things to improve on a CSS side (already noticed the carousel arrows) and the arabic content can probably be improved.

There's not much more for now, I'm working on a full test page using every components: I think your experience will be extremely valuable when it's here, to find common issues and help fixing them.

Also, we'll need to document working with RTL, so feel free to share any ressource you wrote or any suggestion that may help people using Bootstrap to work with RTL: there're tons of things missing on this PR current roadmap, I guess.

Speaking about fallback for logical properties, does this mean physical properties? Or do you have another stratégie?

@ahmadalfy

Copy link
Copy Markdown

Hi @ffoodd

Will definitely check and share feedback if I have any. I can help with the documentation part for sure. Regarding the resources I wrote this article recently (Building Multi-Directional Layouts) about logical properties and values and how PostCSS plugins can help you use them now. I wrote another piece 6 years ago that still stands firm as well (Let's talk about RTL).

There is another excellent guide written by Ahmad Shadeed RTLStyling.com. It covers a lot more topics and can be considered a complete resource for anyone who wants to work with RTL languages.

Regarding your question about logical properties and values; yes, the fallback is done using physical properties. It's automatically translated using this PostCSS plugin.

Please feel free to ping me anytime you need anything.

@Tawfeekamr

Copy link
Copy Markdown

Hello there... I'm Tawfeeq Amro working with biggest Arabic website in World (www.mawdoo3.com), and I hope I can help you in Arabic proofreading, Arabic translation, technicality and more.

@kamel3d

kamel3d commented Jun 21, 2020

Copy link
Copy Markdown

Hi,
I am a graphic designer (working for TV ) and web developer in my part-time, I could help you with Arabic translations and terminology since I was working on some projects online to improve the presence of the Arabic language on the web

@badrshs

badrshs commented Jun 22, 2020

Copy link
Copy Markdown

@ffoodd one of the problem that I found in your example in this url
https://deploy-preview-30980--twbs-bootstrap.netlify.app/docs/5.0/examples/carousel-rtl/

image

the arrows should point to outside not inside .. like this < ---- >

@Tawfeekamr

Copy link
Copy Markdown

@ffoodd one of the problem that I found in your example in this url
https://deploy-preview-30980--twbs-bootstrap.netlify.app/docs/5.0/examples/carousel-rtl/

image

the arrows should point to outside not inside .. like this < ---- >

This problem can be fixed by reflecting arrows transform: scale(-1);

image

or changing arrows positions (Better browser support):

.rtl .carousel-control-prev { left:0; }

.rtl .carousel-control-next { right:0; }

image

@badrshs

badrshs commented Jun 22, 2020

Copy link
Copy Markdown

or changing arrows positions (Better browser support):
thanks @Tawfeekamr

yep the second solution is better I think

@ahmadalfy

Copy link
Copy Markdown

or changing arrows positions (Better browser support):
thanks @Tawfeekamr

yep the second solution is better I think

That's actually a worse solution. This way you are moving the back button to the far left; which in Arabic should be on the right. What should be done is either using transform like @Tawfeekamr offered or to switch the pseudo content value of bootstrap font in Arabic.

@ffoodd ffoodd force-pushed the master-fod-rtlcss branch from 204c395 to cf9a584 Compare November 6, 2020 10:29

@Johann-S Johann-S left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all for me, nice (and huge) work @ffoodd 👍

Comment thread js/src/modal.js
Comment thread js/tests/unit/tooltip.spec.js Outdated
Comment thread js/src/tooltip.js Outdated
@ffoodd

ffoodd commented Nov 14, 2020

Copy link
Copy Markdown
Contributor Author

Great! Will take care of it, thanks 🙂
@Johann-S did you check modal's behaviour? There're plenty padding related things and I'm not sure what's needed for RTL. I fixed a single case but there might be more, I'm not confident at all with this component ATM.

@Johann-S

Copy link
Copy Markdown
Member

@ffoodd yep I checked as much as possible use case and it works fine to me 👌

@ffoodd

ffoodd commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

@Johann-S Need help :') I added the left placement test for tooltips, but it keeps failing with Karma.

When logging tooltipShown.classList in the test, it's using .bs-tooltip-end on the shown event (but is correct on the the inserted event) and I don't understand what's happening. This is litteraly working so I'm not sure of what's going on with this test 🤔

@Johann-S

Copy link
Copy Markdown
Member

@ffoodd I think you went to far, you tried to test updateAttachment on show unit tests.

updateAttachment is a separate method you can test it separately.

The class changed because if Popper.js detect not enough spaces it change the tooltip position

@ffoodd

ffoodd commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

Ok I think I'm done here, still need to review all the things but it's definetely better. I'll try to check deeply tomorrow!

@Johann-S I updated my test to target updateAttachment() and stop on what's inserted, since it's what's relevant here.

@XhmikosR I did my best to clean up patches, let me know if something is still wrong.

Comment thread js/src/carousel.js Outdated
const CLASS_NAME_SLIDE = 'slide'
const CLASS_NAME_RIGHT = 'carousel-item-right'
const CLASS_NAME_LEFT = 'carousel-item-left'
const CLASS_NAME_RIGHT = 'carousel-item-end'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might as well rename the constants here and in any other places that makes sense :) I'll have a look later

@ffoodd

ffoodd commented Nov 24, 2020

Copy link
Copy Markdown
Contributor Author

@ahmadalfy @devmium @Tawfeekamr @kamel3d @badrshs This is ready and only requires review, should land in v5-beta1. If you have any time to review and contribute after the merge, I'd be glad to have some feedback on this long-lasting work :)

@Tawfeekamr

Tawfeekamr commented Nov 24, 2020

Copy link
Copy Markdown

beta1

Dear @ffoodd ... I can follow this link to make a review?

https://deploy-preview-30980--twbs-bootstrap.netlify.app/docs/5.0/examples/carousel-rtl/

Also, There are translation issues, I can report them here?

@ffoodd

ffoodd commented Nov 24, 2020

Copy link
Copy Markdown
Contributor Author

Hi @Tawfeekamr,

The best place to check is RTL cheatsheet.

You may report any issue here or open a dedicated issue if it's easier for you 🙂

Thanks for your help!

@Tawfeekamr

Tawfeekamr commented Nov 24, 2020

Copy link
Copy Markdown

Hi @Tawfeekamr,

The best place to check is RTL cheatsheet.

You may report any issue here or open a dedicated issue if it's easier for you 🙂

Thanks for your help!

Okay, no problem...

1.Never say "معاق" for "Disabled" inputs, the best translation from my opinion is "معطل".

google take the translation in the wrong way:
Disabled: (of a person) having a physical or mental condition that limits movements, senses, or activities.

  1. Carousel slider must move from right to left in RTL (I mean the auto move from slide to another).

  2. Carousel means "دائري" in google translate, this translation does not make any sense! the correct one to get the meaning of Carousel/slider is "شرائح عرض" or "شرائح عرض متحركة".
    Again "دائري" means in Arabic "Circular" if you mean that meaning, keep it! but if you mean slider I think "شرائح عرض" closer to the Arabic developer.

@mdo

mdo commented Dec 1, 2020

Copy link
Copy Markdown
Member

We can save some of these outstanding todos for Beta 2. I just pushed some changes to clean up the examples for now. I'd like to add a few things to the list if it makes sense:

  • Move cheatsheet from examples to main docs
  • Allow main docs to be passed a direction in the front matter and have that automatically toggle LTR to RTL CSS inclusion
  • The above items should resolve most of this, but... De-dupe the RTL CSS once moved
  • Convert page content to Markdown

@ffoodd

ffoodd commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

@mdo Setting the dir in our docs is very easy (did it for examples) however that means we'll need to convert our docs CSS to RTL too. And since we (probably) won't translate our navigations et all, it means our RTL cheatsheet would be in arabic in an english page?

So either we don't use dir on the whole page and simply set dir and lang on main content (and ToC), or we drop the arabic translation and only use dir on the main content… which would be quite deceptive.

I started with the cheatsheet in our docs and struggled with more questions (comment), and following @devmium reply (comment) I decided to go for an example.

As mentionned in my linked comment, I almost managed to create a shortcode that parsed our markdown content to get and instantiate every occurrencies of our example shortcode, so it can be automated pretty easily ‑ but without any kind of translation. However that's another concern, I guess.

Or we add the cheatsheet in the main docs, but leave the RTL counterpart in examples. Would still require manual maintenance though.

@MartijnCuppens MartijnCuppens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Well done!

@FaridAghili

Copy link
Copy Markdown

@ffoodd
In this url, please take a look at this:
image

I guess we should flip this chevrons like this:

image

Which can be done with transform: rotate(180deg):
image

Also the transform-origin can be increased to .6em to have more space between chevron and text.

@XhmikosR

XhmikosR commented Dec 4, 2020

Copy link
Copy Markdown
Member

@FaridAghili please make a new issue, this PR is long enough as is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet