joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

Bootstrap RTL the correct way

Open brianteeman opened this issue 3 years ago • 21 comments

This PR is using bootstrap the way that bootstrap intended for it to be used since 5.0 for RTL.

removing any rtl specific code that can be automatically handled by rtlcss or by using rtlcss specific markup (just as bootstrap itself does)

All our current css is doing bootstrap the wrong way for rtl support.

The problem is NOT with our own scss as in general this is now correct with logical properties (The few instances of dir=rtl are now handled by rtlcss)

The problem is with the actual bootstrap code. We are using it without addressing any RTL issues with that. With the exception of a very incomplete set of bootstrap overrides in https://github.com/joomla/joomla-cms/blob/7cf5e1228db337881d7678ad1c71915d5c885150/build/media_source/templates/administrator/atum/scss/vendor/bootstrap/_bootstrap-rtl.scss

Technically this will change all left to right etc so we dont need to use logical properties in our own css however I still think its good practice that we continue to use logical properties where available.

Bootstrap 5 is designed to be post-processed using rtlcss - This takes the generated template.css and converts it to template-rtl.css

Benefits

  • it is DRY
  • we don't need to remember to check it works in rtl (we almost always forget)
  • we dont need to waste time creating rtl overrides (in most cases)
  • fixes numerous open issues
  • replaces numerous open pull requests
  • fixes several issues with input buttons not already on the tracker.

Testing

  1. Apply the pr and then run npm i This will generate all the css and install rtlcss

Or

2.Use a prebuuilt package

  1. test test test

Sample Tests

Global configuration in arabic

Before

image

After

image

Add Multi-Factor Authentication

Before

image

After

image

brianteeman avatar Aug 07 '22 10:08 brianteeman

With this PR we need to update the build tools. rtlcsss is a postcss plugin that runs on the generated template.css of atum and cassiopeia and after generating the new -rtl.css this needs to be minified as well.

https://github.com/brianteeman/joomla-cms/pull/291

dgrammatiko avatar Aug 07 '22 21:08 dgrammatiko

Thanks @dgrammatiko

brianteeman avatar Aug 08 '22 09:08 brianteeman

Thought it was way more work than this.

Well it was 14hours solid so not a small amount - plus dont forget all the PR since you did the first work where I moved our own css to logical properties

brianteeman avatar Aug 09 '22 11:08 brianteeman

Yeah that makes a lot of sense. Still really nice work though :)

wilsonge avatar Aug 09 '22 11:08 wilsonge

Thanks

brianteeman avatar Aug 09 '22 12:08 brianteeman

is there any point in me fixing the conflicts here or is it just going to be ignored like so many pull requests recently?

brianteeman avatar Sep 09 '22 09:09 brianteeman

Your PR's are not ignored .. there are only not enough active testers.

chmst avatar Sep 09 '22 11:09 chmst

This should be rebased to 4.3 anyway.

laoneo avatar Sep 09 '22 11:09 laoneo

its a bug fix not a feature

brianteeman avatar Sep 09 '22 11:09 brianteeman

19 changed files and a new package is a too big change for a bug fix release. I welcome the change, but we really need to bring 4.2 into a stable state, so stuff like this needs to go into the next minor. I know we both have different opinions what a bug fix is, but we should start to handle patch releases as such ones and not getting the risk to become unstable again, even when the chance is small. I really hope you understand that point, this here is not a risky one. Again, the change is very welcome but please for 4.3.

laoneo avatar Sep 09 '22 12:09 laoneo

rebased as requested - of ciurse its now untestable until 4.3 brnch is brought up to date

brianteeman avatar Sep 09 '22 12:09 brianteeman

Thanks, merging stuff like this as early as possible brings us more time to iron out regressions, even when the chance is small that there will be one.

laoneo avatar Sep 09 '22 12:09 laoneo

except it cant be tested right now :(

Thanks, merging stuff like this as early as possible brings us more time to iron out regressions, even when the chance is small that there will be one.

I hope this means that pull requests wont wait as RTC for weeks and months before merging any more

brianteeman avatar Sep 09 '22 12:09 brianteeman

Updated the branch, should be back to normal.

laoneo avatar Sep 10 '22 07:09 laoneo

Thanks @laoneo

brianteeman avatar Sep 10 '22 08:09 brianteeman

I have tested this item :white_check_mark: successfully on 4609b33164939f64dbe574aeade304388d129735

Tested it with farsi. Did browse around and saw no issue. Installed DPCalendar and created events and so on. All did look ok with RTL.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38412.

laoneo avatar Sep 16 '22 13:09 laoneo

thanks for testing. dpc was one of the components I developed this with ;)

brianteeman avatar Sep 16 '22 16:09 brianteeman

I have tested this item :white_check_mark: successfully on 95ba902e072a87d0aeed8c3f1acf21ad1f8e657b

I did extensive browsing trying to catch issues. As a non-rtl reader, I may have missed issues related to rtl that I may not be unaware of, but I honestly think this PR was a great work through. It would be useful to get a tester using rtl daily.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38412.

obuisard avatar Sep 18 '22 17:09 obuisard

made some tests, but i'm not a RTL user... ...so if we don't get any RTL tester... you can consider this as a valid one

alikon avatar Sep 19 '22 08:09 alikon

fyi I use RTL a lot which is why I spent the time making the pr

brianteeman avatar Sep 19 '22 08:09 brianteeman

I would like to merge this PR into 4.3 early on. I will merge it tomorrow if there are not any issues by then.
Any objection @chmst @HLeithner ?

obuisard avatar Sep 20 '22 15:09 obuisard

is it tomorrow yet?

brianteeman avatar Sep 23 '22 20:09 brianteeman

Was hoping to hear from reviewers...

obuisard avatar Sep 23 '22 21:09 obuisard

Thank you Brian @brianteeman for this needed fix and for keeping track.

obuisard avatar Sep 23 '22 21:09 obuisard