[#38412] - Bootstrap RTL the correct way
- Fixed in Code Base
- 23 Sep 2022
- Medium
- Build: 4.2-dev
- # 38412
- Diff
- brianteeman:bootstrap_rtl
User tests: Successful: 0 Unsuccessful: 0
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
- Apply the pr and then run
npm i
This will generate all the css and install rtlcss
Or
2.Use a prebuuilt package
- test test test
Sample Tests
Global configuration in arabic
Before
After
Add Multi-Factor Authentication
Before
After
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Repository NPM Change |
| Labels |
Added:
NPM Resource Changed
?
|
||
| Title |
|
||||||
| Category | Repository NPM Change | ⇒ | JavaScript Repository NPM Change |
| Title |
|
||||||
Thanks @dgrammatiko
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
Yeah that makes a lot of sense. Still really nice work though :)
Thanks
is there any point in me fixing the conflicts here or is it just going to be ignored like so many pull requests recently?
Your PR's are not ignored .. there are only not enough active testers.
This should be rebased to 4.3 anyway.
its a bug fix not a feature
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.
| Category | Repository NPM Change JavaScript | ⇒ | Repository Administration com_banners com_categories com_contact com_content com_cpanel com_fields com_finder com_languages com_media com_menus com_modules com_newsfeeds com_redirect com_tags com_templates com_users |
rebased as requested - of ciurse its now untestable until 4.3 brnch is brought up to date
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.
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
| Labels |
Added:
PR-4.3-dev
Removed: NPM Resource Changed |
||
| Category | Repository Administration com_banners com_categories com_contact com_content com_cpanel com_fields com_finder com_languages com_media com_menus com_modules com_newsfeeds com_redirect com_tags com_templates com_users | ⇒ | JavaScript Repository NPM Change |
Updated the branch, should be back to normal.
I have tested this item
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.
thanks for testing. dpc was one of the components I developed this with ;)
| Labels |
Added:
NPM Resource Changed
Removed: ? |
||
I have tested this item
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.
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
fyi I use RTL a lot which is why I spent the time making the pr
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 ?
is it tomorrow yet?
Was hoping to hear from reviewers...
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-09-23 21:11:38 |
| Closed_By | ⇒ | obuisard |
Thank you Brian @brianteeman for this needed fix and for keeping track.
brianteeman#291