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

Problem w change in media query processing #16

Closed
mschipperheyn opened this issue Mar 17, 2017 · 8 comments
Closed

Problem w change in media query processing #16

mschipperheyn opened this issue Mar 17, 2017 · 8 comments

Comments

@mschipperheyn
Copy link

In this merge b832bb2 the workings of styled flexbox grid have been changed.

I don't think this is a good choice, and I'm having problems in my personal project because of this.

First, the merge changes em processing to px processing. This is not the way flexboxgrid (https://github.com/kristoferjoseph/flexboxgrid/blob/master/src/css/flexboxgrid.css) processes units and makes react-styled-flexboxgrid actually not flexboxgrid anymore.

The +1 on the min width was presumably the reason to switch to pixels, since this is the common way to prevent media query problems. On my project I'm seeing single columns on two <Col xs={12} sm={6} [...] because of this.

In short, this can't be right.

@LoicMahieu
Copy link
Owner

LoicMahieu commented Mar 17, 2017

Hi
First, thanks for your feedback.
The motivation behind #13 has not been explain and it's a shame ;) I didn't handle it directly so I can't explain clearly why we did this. I wasn't confortable with the fact to go to pixels values too.

I will come back here next week with the explanation and we will discuss about it.

@mschipperheyn
Copy link
Author

Perhaps you could add the breakpoint unit type to the theme definition so that people can customize it, if they want that, but otherwise functionality is preserved

@tlvenn
Copy link

tlvenn commented Mar 28, 2017

As illustrated in the following article, em should be used in breakpoints to work properly cross-browser and support users changing their browsers font-size

https://zellwk.com/blog/media-query-units/

@zentuit
Copy link

zentuit commented Mar 29, 2017

If you read the comments in that article, you'll see evidence from folks that the author is incorrect in his conclusions - specifically the Safari not reporting px the same as the other browsers when zooming. If you look at the animated gif in his article, you'll see that the px test matches the other browsers' tests; its the em/rem tests that differ on Safari. Px based queries get consistent results across browsers when zooming.

I just tested this on Chrome and Safari and both Chrome and Safari's media query with px behave the same. You can try with http://codepen.io/blyne/pen/mPwmvX (I changed from the 400px/25em/25rem to 640px/40em/40rem)

@tlvenn
Copy link

tlvenn commented Mar 30, 2017

Good point @zentuit , i have overlooked the comments.

A media query using em or rem (e.g. max-width) is based on the size of the viewport.
The viewport isn't affected by the elements inside of it even if the elements are larger than the viewport.
CSS gives us control over the font sizes of the elements, but not the viewport.

I guess that would explain the switch from em to px ?

@mschipperheyn
Copy link
Author

Let's also not forget that this library is supposed to be a react implementation of flexboxgrid, which clearly goes the "em" way

@splars
Copy link

splars commented Apr 20, 2017

Thanks for creating this lib, I think it's really easy to use. Is there any plan to deal with this issue?

@LoicMahieu
Copy link
Owner

Sorry to being back late.
@mschipperheyn @splars I agree with you, this library is supposed to be kind of port of flexboxgrid and I want to follow their implementation.

I reverted the commit in #35 and it is published v1.1.0

Thanks

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

No branches or pull requests

5 participants