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

Bootstrap grid - Spans wrapping between 768px - 979px #13768

Closed
ciar4n opened this issue Jan 26, 2017 · 14 comments
Closed

Bootstrap grid - Spans wrapping between 768px - 979px #13768

ciar4n opened this issue Jan 26, 2017 · 14 comments

Comments

@ciar4n
Copy link
Contributor

ciar4n commented Jan 26, 2017

The Bootstrap responsive grid has some issues between 768px and 979px browser width. Spans/columns wrap when they should remain inline.

The issue appears to be due to an incorrect margin-left calculation (@fluidGridColumnWidth768)... https://github.com/joomla/joomla-cms/blob/staging/media/jui/less/variables.less#L273-L300

In Isis, commenting out each of the following does resolve the issue however before I go creating a PR, I would appreciate some further thoughts from anyone with a better understand of each of these less files. I realise they are changing the gutter width however they appear to be doing so incorrectly for this instance. If they can't be fixed maybe it would be best to remove them..

https://github.com/joomla/joomla-cms/blob/staging/administrator/templates/isis/less/template.less#L62
https://github.com/joomla/joomla-cms/blob/staging/administrator/templates/isis/less/template.less#L64

EDIT: On further inspection, comparing versions compiled by lessc and an alternative compiler, it is the span widths that are incorrect rather than the margin-left.. https://github.com/joomla/joomla-cms/blob/staging/administrator/templates/isis/css/template.css#L352-L399

Steps to reproduce the issue

Navigate to article edit and reduce the browser window. Columns wrap prematurely..

Expected result

grid2

Actual result

grid1

System information (as much as possible)

Additional comments

@mbabker
Copy link
Contributor

mbabker commented Jan 26, 2017

I don't think it's the LESS files. I think it's the compiler. I have fought with similar grid related issues on the joomla.org site template and ultimately said "screw it" and changed to using Bootstrap's old recess compiler versus the lessc library. Fixed all of my issues.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 27, 2017

You have mentioned this before but only now checking it out for myself.. using an alternative compiler does indeed resolve the issue. Considering this and presuming changing the compile method won't fly maybe getting Isis to load it's own version of responsive-768px-979px.less might be the best option. We can replace the calculated variables with the correct static values, making it less likely for lessc to mess up?

@Bakual
Copy link
Contributor

Bakual commented Jan 27, 2017

Can't we fix the lessc compiler upstream then? I have no clue where the issue comes from but sounds like the right approach to fix it 😄

@C-Lodder
Copy link
Member

It's over 2 years old. Perhaps consider using a more modern solution?

@dgrammatiko
Copy link
Contributor

I could write a small grunt script for compiling less to CSS and .min.css if that's ok for 3.x

@C-Lodder
Copy link
Member

That would mean everyone having to download Node. I wouldn't suggest that for a minor update. It also doesn't really help the end user who may want to make LESS changes via the Template Manager

@dgrammatiko
Copy link
Contributor

The other option is to switch to https://github.com/oyejorge/less.php which apparently also seems outdated...

@C-Lodder
Copy link
Member

or a JS library

@mbabker
Copy link
Contributor

mbabker commented Jan 28, 2017

That less.php library doesn't have a compatible license for us to ship it. We got away with it for Bootstrap but I doubt it would fly for a PHP library.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 31, 2017

I think the issue with lessc is what is suggested here.. leafo/lessphp#352

Personally I think compiling with Node (for Isis at least) is the way to go. If a user is familiar with LESS, the chances are they are also familiar with using Node.

@C-Lodder
Copy link
Member

I wouldn't agree with making people have to use Node to compile in a minor version update. Should stick to 4.x for that

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 31, 2017

After that then the only likely alternative is to override the lessc output with a corrected version in the template.less?

@mbabker
Copy link
Contributor

mbabker commented Jan 31, 2017

I don't know what's being cached by lessc, but I know something is. Whether it's the mixin results, variable values, or something else, I have no idea. But I know it has resulted in me labeling that library as unusable.

@ciar4n
Copy link
Contributor Author

ciar4n commented Feb 1, 2017

PR replacing the values calculated by lessc with the correct values.. #13845

@ciar4n ciar4n closed this as completed Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants