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

Let's simplify .col-full selectors #551

Open
gorgoglionemeister opened this issue Feb 20, 2017 · 6 comments
Open

Let's simplify .col-full selectors #551

gorgoglionemeister opened this issue Feb 20, 2017 · 6 comments
Labels
type: enhancement The issue is a request for an enhancement.

Comments

@gorgoglionemeister
Copy link

gorgoglionemeister commented Feb 20, 2017

Reading the stylesheets, I found that the .col-full selectors are a mess. The rules:

@include susy-media($desktop) {
	.col-full {
		@include clearfix;
		@include container($container-width);
		padding: 0 ms(5);
		box-sizing: content-box;
	}

@include susy-media (max-width $container-width) {
	.col-full {
		margin-left: ms(5);
		margin-right: ms(5);
		padding: 0;
	}
}

@include susy-media (max-width $handheld) {
	.col-full {
		margin-left: ms(2);
		margin-right: ms(2);
		padding: 0;
	}
}

In contrast with the rest of the code, .col-full is styled with a desktop-first approach, making it difficult to understand its behavior.

So, there is .col-full between ∞ and 67.142em, a variant between 67.141em and 768px, a variant between 767px and 569px and another one between 568px and 0.

Why not simplify things in a mobile-first way, removing the max-width media queries?

@gorgoglionemeister gorgoglionemeister changed the title Let's simplify the .col-full selectors Let's simplify .col-full selectors Feb 21, 2017
@gorgoglionemeister gorgoglionemeister changed the title Let's simplify .col-full selectors Let's simplify .col-full selectors Feb 21, 2017
@jameskoster
Copy link
Member

I agree this could be simplified.

@jameskoster jameskoster added this to the Future release milestone Feb 22, 2017
@gorgoglionemeister
Copy link
Author

May I ask where the value of the variable $container-width, in the stable version, 67.141em, comes from?

@jameskoster
Copy link
Member

It's left over from the modular scale we used originally. We should probably update that to use the new ms() sass function :)

jameskoster added a commit that referenced this issue Feb 27, 2017
Also rearranges the order that vendor styles are included in style.scss
so that variables.scss has access to ms()
@gorgoglionemeister
Copy link
Author

$container-width can not have peace :) I don't like it bounded to a modular scale step, but OK. Is there a way to overwrite this value (and other media queries) in a child theme?

@jameskoster
Copy link
Member

I don't like it bounded to a modular scale step

How come? Everything else is.

Is there a way to overwrite this value (and other media queries) in a child theme?

I'm afraid not :( If you can think of a way however a PR would be most welcome :)

@gorgoglionemeister
Copy link
Author

I mean, modular scale is OK for vertical spaces, but I don't like it for horizontal ones. In my opinion, 60em (960px) is a more practical value than 66.4989378333em (1063.9830053328px).

@jameskoster jameskoster added the type: enhancement The issue is a request for an enhancement. label Oct 2, 2017
@jameskoster jameskoster modified the milestones: Future release, 2.3.0 Oct 2, 2017
@tiagonoronha tiagonoronha removed this from the 2.3.0 milestone Jan 4, 2018
@tiagonoronha tiagonoronha removed their assignment Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

No branches or pull requests

3 participants