Skip to content

Conversation

@brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Aug 7, 2022

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

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 or dir=rtl

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

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

This PR is my draft where I am working through all the current scss and either removing any rtl specific code that can be automatically handled by rtlcss or by using rtlcss specific markup (just as bootstrap itself does)

## Benefits
- it is DRY
- we don't need to remember to check it works in rtl (we almost always do)
- fixes numerous open issues
- replaces numerous open pull requests
- fixes several issues with input buttons not already on the tracker.
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Aug 7, 2022
@brianteeman brianteeman changed the title Bootstrap RTL the correct way WORK IN PROGRESS Bootstrap RTL the correct way - testable - but help wanted Aug 7, 2022
@brianteeman brianteeman marked this pull request as ready for review August 7, 2022 19:01
@dgrammatiko
Copy link
Contributor

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.

brianteeman#291

dgrammatiko and others added 4 commits August 7, 2022 23:08
Requires an entry point in the templates ending with `-rtl.scss`, eg `template-rtl.scss`
@brianteeman brianteeman changed the title Bootstrap RTL the correct way - testable - but help wanted Bootstrap RTL the correct way Aug 8, 2022
@brianteeman
Copy link
Contributor Author

Thanks @dgrammatiko

Copy link
Contributor

@wilsonge wilsonge left a comment

Choose a reason for hiding this comment

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

AMAZING if this works. I looked when I plumbed in 5.x and it seemed really painful to migrate everything. Thought it was way more work than this. Really nice job!

@brianteeman
Copy link
Contributor Author

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

@wilsonge
Copy link
Contributor

wilsonge commented Aug 9, 2022

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

@laoneo
Copy link
Member

laoneo commented Sep 9, 2022

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.

@brianteeman
Copy link
Contributor Author

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

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Sep 10, 2022
@laoneo
Copy link
Member

laoneo commented Sep 10, 2022

Updated the branch, should be back to normal.

@brianteeman
Copy link
Contributor Author

Thanks @laoneo

@laoneo
Copy link
Member

laoneo commented Sep 16, 2022

I have tested this item ✅ successfully on 4609b33

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.

@Quy Quy removed the PR-4.2-dev label Sep 16, 2022
@brianteeman
Copy link
Contributor Author

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

@obuisard
Copy link
Contributor

obuisard commented Sep 18, 2022

I have tested this item ✅ successfully on 95ba902

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.

@alikon
Copy link
Contributor

alikon commented Sep 19, 2022

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

@brianteeman
Copy link
Contributor Author

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

@obuisard obuisard added the RTC This Pull Request is Ready To Commit label Sep 20, 2022
@obuisard
Copy link
Contributor

obuisard commented Sep 20, 2022

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 ?

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 20, 2022
@brianteeman
Copy link
Contributor Author

is it tomorrow yet?

@obuisard
Copy link
Contributor

Was hoping to hear from reviewers...

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Sep 23, 2022
@obuisard obuisard merged commit a396654 into joomla:4.3-dev Sep 23, 2022
@obuisard
Copy link
Contributor

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

@brianteeman brianteeman deleted the bootstrap_rtl branch September 23, 2022 21:55
richard67 added a commit to richard67/joomla-cms that referenced this pull request Oct 29, 2022
Update to changes from following PRs:
- joomla#38353
- joomla#38412
- joomla#38823

Remove wrong '/build/media_source ...' added with PR joomla#38353 .
wilsonge pushed a commit that referenced this pull request Oct 30, 2022
Update to changes from following PRs:
- #38353
- #38412
- #38823

Remove wrong '/build/media_source ...' added with PR #38353 .
@brianteeman brianteeman mentioned this pull request Dec 6, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants