Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Hard coded spaces #32244

Merged
merged 1 commit into from
Feb 3, 2021
Merged

[4.0] Hard coded spaces #32244

merged 1 commit into from
Feb 3, 2021

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Feb 2, 2021

It's 2021 and we really should be using css and not spaces for layout. This example PR is for the language layout and replaces the hard coded space with the correct css class of me-1. (because of the new BS5 classes this is RTL aware)

image

If accepted there are plenty of places to update

It's 2021 and we really should be using css and not spaces for layout. This example PR is for the language layout and replaces the hard coded space with the correct css class of me-1. (because of the new BS5 classes this is RTL aware)
@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2021

@ciar4n can I get your steer on this please. I don’t have enough understanding of pros/cons to make an informed enough decision

@brianteeman
Copy link
Contributor Author

The only reason I can think of for why the space was misused for layout was that prior to bootstrap 5 it would have needed different classes for LTR and RTL

@ciar4n
Copy link
Contributor

ciar4n commented Feb 3, 2021

@wilsonge This looks ok to me. Best to avoid hardcoded spaces for spacing items.

On a side note, afaik we are not loading BS5's bootstrap.rtl.css but the changes in #32166 cover this in RTL.

@brianteeman
Copy link
Contributor Author

Thanks @ciar4n it looks like it was just done to avoid using css as the majority of instances of using a space for layout are all related

@wilsonge
Copy link
Contributor

wilsonge commented Feb 3, 2021

Then go for it

@brianteeman
Copy link
Contributor Author

I will do when I get the opportunity. Doesnt hold back the merge of this PR

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 2ba5ac9


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

@infograf768
Copy link
Member

Note while testing this in RTL:
I remarked the same problem as #32231 in many list where the width of some columns depends on the length of the title of the column. It is specially obvious when one uses Persian language.

Screen Shot 2021-02-03 at 12 14 36

Screen Shot 2021-02-03 at 12 15 00

Any volunteer to make general PR?

PS: Yes, I know it is unrelated to this PR.

brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Feb 3, 2021
Similar to joomla#32244 this p[r replaces a hard coded space with a margin. In addition it removes text-center from the column to aid readability
@Quy
Copy link
Contributor

Quy commented Feb 3, 2021

I have tested this item ✅ successfully on 2ba5ac9


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

@Quy
Copy link
Contributor

Quy commented Feb 3, 2021

RTC


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

@Quy Quy removed the PR-4.0-dev label Feb 3, 2021
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 3, 2021
@Quy Quy added the PR-4.0-dev label Feb 3, 2021
@Quy Quy added this to the Joomla 4.0 milestone Feb 3, 2021
@drmenzelit drmenzelit merged commit 4554671 into joomla:4.0-dev Feb 3, 2021
@drmenzelit
Copy link
Contributor

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 3, 2021
@brianteeman
Copy link
Contributor Author

thank you

@brianteeman brianteeman deleted the spaces branch February 3, 2021 15:50
Quy pushed a commit that referenced this pull request Feb 4, 2021
Similar to #32244 this p[r replaces a hard coded space with a margin. In addition it removes text-center from the column to aid readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants