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

Add Autoprefixer #10

Merged
merged 2 commits into from Aug 1, 2017
Merged

Add Autoprefixer #10

merged 2 commits into from Aug 1, 2017

Conversation

malchata
Copy link

@malchata malchata commented Aug 1, 2017

#8

  • Added autoprefixer.
  • Added a PostCSS plugin to transform CSS variables to static references for maximum compatibility. This also appears to have reduced the size of the framework significantly.

Be sure to test and make sure that this doesn't introduce regressions. All looks well to me after a spot check, but there could be more that I might have missed.

- Added autoprefixer.
- Added a PostCSS plugin to transform CSS variables to static references for maximum compatibility. This also appears to have reduced the size of the framework significantly.
@claviska
Copy link
Member

claviska commented Aug 1, 2017

Added a PostCSS plugin to transform CSS variables to static references for maximum compatibility. This also appears to have reduced the size of the framework significantly.

To clarify, this removes CSS variables from the dist? If so, that's a bad thing :(

We want to keep it customizable so folks can load the lib via CDN and customize it in their own stylesheet.

@malchata
Copy link
Author

malchata commented Aug 1, 2017

It does remove variables in the compiled output by converting instances of them to their assigned values, yes. It is your project, and I ultimately respect whichever road you'd like to go down, but some thoughts:

  1. CSS variable support is good but not near-absolute: http://caniuse.com/#feat=css-variables
  2. Converting variables to static references at compile time lowers the size of the library to 18.8 KB (again, this may need regression testing).
  3. If users want to customize, they could potentially clone the repo, edit the variables in the pre-processed CSS, and go with a custom build.

That said, I understand configurability is a big goal for this project, so I'll go any way you want. I respect your vision for this and support whatever you want to do. If you want to reject this PR, I can submit a new PR for just the autoprefixer changes.

@claviska
Copy link
Member

claviska commented Aug 1, 2017

I realize CSS vars (and CSS Grid for that matter) are bleeding edge features, but that's part of the vision. The ability to load the lib via CDN and customize it instantly is part of the allure, and I can't imagine giving that up to save a few KB. We'd end up with exactly the same problem I described here.

That said, I recognize a lot of people won't be able to use Shoelace as-is in production today because they still target IE and other older browsers. That will change soon. In the meantime, Myth or cssnext can be added to address that for their particular needs — but I'd prefer to leave that up to the end user to decide on a per-project basis.

Personally, I'm using Shoelace along with Myth (or possible cssnext) to build out Surreal CMS 6.0. I wish I could use it via CDN, but I realize I still have users who are stuck with those older browsers.

Moving forward, I think the dist should remain customizable as Shoelace will only become more and more relevant as these older browsers die out. 😀

@claviska
Copy link
Member

claviska commented Aug 1, 2017

Great work on Autoprefixer, btw. If you want to update the PR I'll get it merged 👍

@malchata
Copy link
Author

malchata commented Aug 1, 2017

Agreed, and I want to adhere to your vision for this project, so I will remove the PostCSS variable package. I'll have a PR in soon.

@claviska
Copy link
Member

claviska commented Aug 1, 2017

Awesome. I appreciate the effort too. I'll probably revise the website and break it into sections at some point. That will be a great time to add stuff like this (and even sample code) to teach power users how to optimize things. It's just kinda hard with the current one-pager. Didn't think it would blow up like this 😆

@malchata
Copy link
Author

malchata commented Aug 1, 2017

Removed postcss-css-variables from the project. So now it's just autoprefixer in there.

@claviska claviska changed the title Some enhancements: Add Autoprefixer Aug 1, 2017
@pkill37
Copy link

pkill37 commented Aug 1, 2017

Just wanted to say I really agree with the following

In the meantime, Myth or cssnext can be added to address that for their particular needs — but I'd prefer to leave that up to the end user to decide on a per-project basis.

In most cases postprocessing is inevitable for production but this shouldn't be your responsibility, especially since shoelace is a future-proof, standards-compliant library. I would like to see it become a very simple library that deals the latest and greatest vanilla CSS with no complicated setup or build options and nothing else.

Leave postprocessing and all that stuff to the end user, whose project probably has very specific needs and requirements (potentially rendering this autoprefixer configuration and any other processing pointless) and who will probably be using other libraries and will need to minify and optimize everything together anyway.

In short, postprocessing just seems like unnecessary, unrewarding work for this specific library.

@malchata
Copy link
Author

malchata commented Aug 1, 2017

I don't think postprocessing is inappropriate to a desired threshold. The focused optimizations that cssnano provides is, entirely appropriate. Autoprefixing and interpolating static references for CSS variables could certainly be argued against, but I defer to the vision of the project owner.

@claviska
Copy link
Member

claviska commented Aug 1, 2017

I agree that a certain level of postprocessing is absolutely useful, particularly to shrink the overall size and add prefixes. That's why the repo ships with source and dist.

Ultimately, I don't want to lose the ability to customize things out of the box. That's the key feature that makes this project special. And it's not just about setting a couple variables — CSS vars cascade so you can scope overrides to a specific element and do all sorts of fun things:

https://codepen.io/claviska/pen/RZRbqr

This is the future of CSS — not just static vars like Sass/Less/etc. provide. This is where CSS starts to get fun! 🎢

@claviska
Copy link
Member

claviska commented Aug 1, 2017

Look great. Thanks for this!

@claviska claviska merged commit 987ca52 into shoelace-style:master Aug 1, 2017
@claviska claviska mentioned this pull request Aug 1, 2017
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

Successfully merging this pull request may close these issues.

None yet

3 participants