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

Break variables stylesheet up #964

Closed
MichaelArestad opened this issue May 31, 2017 · 3 comments · Fixed by #3804
Closed

Break variables stylesheet up #964

MichaelArestad opened this issue May 31, 2017 · 3 comments · Fixed by #3804
Assignees
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Build Tooling Issues or PRs related to build tooling

Comments

@MichaelArestad
Copy link
Contributor

Currently, the variables are in a single massive junk drawer of a stylesheet. Break them up for more context and simplicity. I would also keep the variables relevant to specific components with those specific components (most of the stuff in the Other section).

You may also look into a breakpoints mixin that avoids pixel overlap, but I trust @mtias work that out if needed.

@youknowriad
Copy link
Contributor

Not exactly related to this, but worth noting that we're using these common stylesheets in blocks, components, editor .... (basically all the modules) but they are declared inside the editor module, we should move them to a global "assets" folder I think.

@nylen
Copy link
Member

nylen commented May 31, 2017

Also semi-related, it looks like we're currently duplicating a lot of stuff across all of our Webpack builds (ref).

In the development build, every single stylesheet (not just every Webpack target) gets all of the comments from _variables.scss added to it. I am not sure if there is more duplication beyond just the comments, but ideally this should only be included once globally.

@MichaelArestad
Copy link
Contributor Author

I would also make most of the comments // this style of comments since they aren't output. The duplication of variables doesn't really matter much unless an expected cascade (or changed variable) is supposed to happen halfway through for some reason (don't do that). Also, I'm pretty sure the comments can be stripped out for the build regardless.

@mtias mtias added [Type] Build Tooling Issues or PRs related to build tooling Good First Issue An issue that's suitable for someone looking to contribute for the first time labels May 31, 2017
@jasmussen jasmussen self-assigned this Dec 4, 2017
jasmussen pushed a commit that referenced this issue Dec 4, 2017
This PR splits out breakpoints and colors from the _variables.scss stylesheet, in a hope to fix #964.

It also uses `//` for comments that should be eaten in compilation.

Further improvements can probably be made if we find either the need for more variables in either of the current sections in _variables.scss, or if we can move some of those variables to live in context of the components that use them. As of now, though, the remaining variables don't feel like enough to split further, and the are used across many components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants