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

Fieldcontain MQ's #5276

Closed
jaspermdegroot opened this Issue Nov 13, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@jaspermdegroot
Member

jaspermdegroot commented Nov 13, 2012

We should change the media queries we use for field containers from min-width 450px to 30em.

@ghost ghost assigned jaspermdegroot Nov 13, 2012

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Nov 13, 2012

Member

I forgot to mention that if we are going to use a class to opt-in MQ's for tables and other responsive features, we should also do the same for field containers.

Member

jaspermdegroot commented Nov 13, 2012

I forgot to mention that if we are going to use a class to opt-in MQ's for tables and other responsive features, we should also do the same for field containers.

@toddparker

This comment has been minimized.

Show comment
Hide comment
@toddparker

toddparker Nov 18, 2012

Contributor

@uGoMobi - I set the breakpoints to 28em, not 30em since 450px divided by 16px = 28.125.

I was going to add the "ui-responsive" class and selector but it seems like the data-role="fieldcontain" is pretty mush the same opt-in to this breakpoint as applying a new class. If you want, we could add the new class selector but maybe just deprecate the data-role="fieldcontain" for 1.3 and drop it in 1.4. Seems like adding a breaking change for such a popular feature strictly for consistency isn't a great tradeoff. Let's discuss that more and open a new ticket if we want to do this.

Contributor

toddparker commented Nov 18, 2012

@uGoMobi - I set the breakpoints to 28em, not 30em since 450px divided by 16px = 28.125.

I was going to add the "ui-responsive" class and selector but it seems like the data-role="fieldcontain" is pretty mush the same opt-in to this breakpoint as applying a new class. If you want, we could add the new class selector but maybe just deprecate the data-role="fieldcontain" for 1.3 and drop it in 1.4. Seems like adding a breaking change for such a popular feature strictly for consistency isn't a great tradeoff. Let's discuss that more and open a new ticket if we want to do this.

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Nov 18, 2012

Member

@toddparker

When I wrote that I was thinking about using the same one or two breakpoints for all widgets and you mentioned 30em (480px) for tables as one of them. As discussed we are not going to use generic breakpoints, so make sense to use 28em here.

I agree that making changes to this feature, just for the sake of consistency is not a good idea. That was not the main reason for suggesting an opt-in class for fieldcontain as well. It's more like, if we introduce this for other widgets it's a good opportunity to do the same here, because it's not easy to negate the fieldcontain CSS if you want to use a different breakpoint.

However, I see your point that adding data-role="fieldcontain" is pretty much the same as adding an opt-in class. Difference is that I added a function to the fieldcontain JS that prevents wrapping of full width forms, so it's not only CSS.

Let's see if we can come up with a solution that doesn't break things when used with current markup.

Member

jaspermdegroot commented Nov 18, 2012

@toddparker

When I wrote that I was thinking about using the same one or two breakpoints for all widgets and you mentioned 30em (480px) for tables as one of them. As discussed we are not going to use generic breakpoints, so make sense to use 28em here.

I agree that making changes to this feature, just for the sake of consistency is not a good idea. That was not the main reason for suggesting an opt-in class for fieldcontain as well. It's more like, if we introduce this for other widgets it's a good opportunity to do the same here, because it's not easy to negate the fieldcontain CSS if you want to use a different breakpoint.

However, I see your point that adding data-role="fieldcontain" is pretty much the same as adding an opt-in class. Difference is that I added a function to the fieldcontain JS that prevents wrapping of full width forms, so it's not only CSS.

Let's see if we can come up with a solution that doesn't break things when used with current markup.

@toddparker

This comment has been minimized.

Show comment
Hide comment
@toddparker

toddparker Nov 18, 2012

Contributor

@uGoMobi - Makes sense. I have been using unique media queries for each widget, but only providing one option via the "ui-responsive" class. If you can come up with a system that doesn't break older markup, that would be ideal. The full width form elements is a wrinkle I hadn't thought of.

BTW - your fieldcontain test pages in /demos/ were really helpful when working on this. It caught a CSS mistake I made right away.

Contributor

toddparker commented Nov 18, 2012

@uGoMobi - Makes sense. I have been using unique media queries for each widget, but only providing one option via the "ui-responsive" class. If you can come up with a system that doesn't break older markup, that would be ideal. The full width form elements is a wrinkle I hadn't thought of.

BTW - your fieldcontain test pages in /demos/ were really helpful when working on this. It caught a CSS mistake I made right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment