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

Change the order of responsive class generation #37

Merged
merged 11 commits into from
Jun 22, 2018

Conversation

balpha
Copy link
Member

@balpha balpha commented Jun 14, 2018

This PR changes the order of responsive class generation such that all .lg:... classes come before all .md:... classes etc., so something like <div class="md:d-none sm:d-block"> is guaranteed to work.

Besides exposing (and fixing) a typo and a missing .sm:mb64 class, the only difference in the generated CSS here is the order of the top/right/bottom/left classes
….` classes come before all `.md:...` classes etc., so something like `<div class="md:d-none sm:d-block">` is guaranteed to work
@aaronshekey
Copy link
Contributor

I ran into an issue while looking at this locally, while running grunt continuously. It appears that our postcss operations are moving the !important outside of the mixin. I've encountered this before, but I wasn't able to find a workaround in that case. 🤔

@aaronshekey
Copy link
Contributor

shellscape/postcss-less#102 This is the relevant issue I'd found.

# Conflicts:
#	docs/assets/css/stacks-documentation.min.css
#	lib/css/stacks.min.css
@aaronshekey aaronshekey changed the title change the order of responsive class generation such that all .lg:... classes come before all .md:... classes etc., so something like <div class="md:d-none sm:d-block"> is guaranteed to work Change the order of responsive class generation Jun 19, 2018
@aaronshekey
Copy link
Contributor

aaronshekey commented Jun 20, 2018

I've spent a good long while poking at this to try to find a workaround. I've tried moving the !important declaration around, even after @rules(), but that hasn't been working.

PostCSS offers us a lot of really nice functionality, both what's implemented today and what could be implemented. Autoprefixing is really nice today, and so is the properties re-ordering, though this is less important.

I see 3 things we can do.

  1. Kill postCSS for now and make autoprefixing a process we do manually.
  2. Continue looking for a workaround for this postcss less bug
  3. Commit a fix upstream for postcss's less plugin. This is above my pay grade.

@balpha
Copy link
Member Author

balpha commented Jun 20, 2018

@aaronshekey As I said in Slack, I see no alternative to dropping postcss (specifically, postcss-less) for now.

Until postcss-less gets support for detached rulesets, a LESS feature that we use a lot, there is bound to be breakage, even if we find a workaround for this particular issue.

And even just for this issue: I've looked at the postcss-less code, and there is no acceptable workaround to prevent this !important moveage that doesn't make our LESS code very awkward to type (and brittle).

What we can do is run the autoprefixer on the generated pure CSS files -- that wouldn't require postcss-less. But for that to work in projects that compile Stacks themselves, in particular Q&A and Talent, these projects would also need to include it, and that might add prohibitively to the build time.

For what it's worth, I'm okay with manual prefixing. These days, vendor prefixes are rarely needed in the browsers we support.

@hellohynes hellohynes added this to Need to fix (Bugs) in Roadmap Jun 21, 2018
Copy link
Contributor

@aaronshekey aaronshekey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with how this is looking with the latest commits 🏆

@balpha balpha merged commit eff8646 into develop Jun 22, 2018
Roadmap automation moved this from Need to fix (Bugs) to Done Jun 22, 2018
@aaronshekey aaronshekey deleted the balpha/responsive-classes-at-the-end branch June 22, 2018 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Roadmap
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants