-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Tools to build CSS from Sass #1149
Comments
The addition of a single
Then, in the cmd line, devs can just run This could also be a way to properly manage things like normalize #1144 |
Here's a working example if anyone wants to paste it into a
|
Not entirely sure if now would be the best time to implement this or to handle this afterwards, but I'd love to see _s including normalize.css again (and not just being based on). |
Thanks for the issue @grappler! This is a good thing to talk about since it's been awhile. I'm up for talking through possible solutions here. Here's some context:
I do like the approach mentioned by @grappler since it's very low key. I'm going to try it out and see how it feels. I'll also take a look at #1159 from @m-e-h. My first questions there is: Do we need all those dependencies? What are they all for? Can it be simpler? Some goals I would have for this:
I'm wondering if we can use Travis and/or another service to compile for us on commit? I don't know if that's possible, but it would be cool to just compile and test locally however people want, and just submit PRs to Sass files. Just an idea. 😄 |
Thank you @davidakennedy for your comments. You have put together a good summary. I would like to add a few points to clarify and not push my idea.
It would be good to make a distinction between contributor and user. I imagine for a contributor it could be easier if we had some standard process instead of needing to having to setup your tool for
Am interested how you find it 😄
We could remove some of the scripts but that would mean typing out the whole commands. Coding Standards are important and we should not exclude them. We currently do not add any extra browser prefixing support so we could exclude
Travis could compile the CSS for us but we would need to figure out how to commit those changes. Potentially we could have underscores.me generate the CSS for us when generating a theme and exclude the CSS from the git repo. As there are number of moving parts we should do it step by step. This is the order that I see it being done.
|
Those are some good thoughts @grappler!
Yes, we have some flexibility here, but if we introduce tools, it adds barriers even if it's a "standard."
I really liked it. It was simple, and the generated file looks really solid.
These steps look good. Ultimately, what I'm after here is a way for the stylesheets to not get out of sync, and for _s to have a sensible build process, if we go that route. Sensible means still easy to get started with, without needing to know a lot of tools. Honestly, I would also be fine with removing Sass altogether. I'd like to do what's best for the majority of our users though, while still making it easy to do what you want. I think the MVP of this could be to have all the maintainers run Also, I'd be interested in using underscores.me to compile the Sass, and potentially build other files, like RTL and POT. All that said, I'd like to wait to see if we get more voices on this before going in a direction. |
Thanks for kicking this off @grappler again. I say again as it's something that as a team we have tried to approach before and not managed to find a path through. Lets find a path! I will be upfront, my initial concern is really that this all is very very dependent on dev tools. In my ideal world the person using _s would not have to do anything node or any other developer tool. We have to be really aware this is not just for developers that are using those tools in the workflow. That said, I think a balance can be reached. My real desire is for as much automation before the user even gets _s. I say user as that is the mindset we should frame thinking about _s in - the one where people are using it as not developers but users. I would therefore strong -1 anything that uses yarn/tweed (joke) /npm or anything at all that requires users to compile. I think it's worth adding how much of a nightmare compiling and being up to date with things like node-sass can be if you are not used to it. I for one have myself fallen down that rabbit hole. I am very keen we don't add these type of hurdles. I have had battles with Travis for example I never want anyone to do - yes I voted against adding that to. I am not super keen on the approach that means any maintainer has to run node-sass to test PRS either. Sorry @davidalankennedy. In the past we have tried to add things like this and it's really hard to regulate. As you point out a lot of the team even at Automattic don't use Sass, or run node anything or even compile any code. I am against at this point .json files because of similar reasons. My vote would be if it could be done without maintainers doing anything or users. That may be an impossible thing to desire. But, I think it's worth exploring. Could it be part of the packaging somehow? I also wouldn't be against removing Sass completely.. if that is an option. Yes I was one of the ones that introduced it, but I don't think it works for this project anymore. |
Thank you @karmatosed for your comments. In relation to your comments I would be interested if you use Sass and if you do what tool you use to compile it.
Don't see
We would not need to require contributors to run
Like I mentioned above this is possible to be included in generation process of the theme but would mean we would remove
Maybe we should start a new discussion to discuss this. |
Now I use it less and less (no pun intended). To backstory, I adore what Sass gives and have in the past used on all themes I created when theming at Automattic, including _s. However, I see Sass as usable nowadays if you:
There are also other options to Sass and I think letting people have whatever works for them with _s works. I also think there are a lot of better options than Sass right now for different situations. This though isn't about my process, it's not about anyone's, this at this point is about what works for _s. Sass I would say doesn't at all right now. If it was happening today I wouldn't be making the PR I did to get Sass into _s. Sass in _s is used incredibly lightly and more as a simple variable manager. We aren't using more than a little of its power and this in itself is a good reason to review. Countless issues of maintaining and complicating our processes to accommodate it, these all add up to me strongly suggesting we remove. |
Yeah I kinda feel like Sass isn't needed for _s. It's not even 900 lines of CSS with Normalize. Variables and mixins aren't used a lot, and it'd be easy to break into partials from the CSS file as-is. I don't see the big value-add that Sass brings to _s and it goes against the un-opinionated philosophy of _s. Doing Sass right also means bringing some complexity (and therefore pain) to _s. We should have a standard Sass build process for maintainers/contributors, but I'm sure many of them use a Git GUI and are unfamiliar with or intimidated by the command line like I was when I started dabbling in Sass. They might be OK using a GUI like Codekit for Sass, but that could generate slightly different compiled CSS than this PR. So maybe Sass isn't worth the trouble of adding a build process or the current process of manually checking and keeping the CSS/Sass in sync. Side note: it would be cool if we had some data about underscores.me downloads using CSS vs. Sass. Is that tracked? Setting that aside, I think @grappler your original suggestion is actually the best approach right now, if we also test for it in Travis. Instead of adding a build process via That to me seems like a good first step before we tackle more controversial changes like adding a |
If I may offer an opinion as a long-time WordPress custom-themer :) I think that removing Sass from _s would be a step backward. While I understand (and agree with) the philosophy that _s should remain as non-opinionated as possible, I don't think that including Sass is opinionated for a couple of reasons:
What I see being discussed here is just the minimum level of tooling to get Sass working. Left on it's own, someone could run with it, which is perfect. Those of us who want something more advanced can quickly base our workflow on it (or likely already have). The bottom line, I think, is that front-end workflows are just complex, and becoming increasingly more complex. Having a pre-processor in place is just the minimum expectation, and, based on my experience, Sass is the most widely used. |
So this issue and the many issues related to it are all basically trying figure out how to best handle the baggage that comes with maintaining SASS and it's compiled CSS. Any methods to manage SASS are of course going to be opinionated and require extra tooling, build processes and dependencies because SASS itself is, more or less, all of those things. The only benefit I see in The variables in The I think the question, "What can we remove?" might be more appropriate here than, "What can we add?". |
We use Grunt in Storefront to help us with several tasks: https://github.com/woocommerce/storefront/blob/master/Gruntfile.js Some of my favourite tasks:
I don't think adding building tools to _s is a bad thing, specially considering that these tools are optional. You can still use Codekit or other apps to compile. I'm also in favour of removing |
I'm not really interesting in giant dev stack to get a working _s theme. I don't use SASS/LESS/etc and therefore need the .css files. |
@sterndata You would still get a CSS file if you used underscores.me to generate the theme if we removed the style.css from the repo. |
OK, I can live with that. :-) Thanks. |
I’m definitely on the side of continuing to use SASS in I know this way could probably come across as too opinionated for some, but for devs or contributors who haven’t made the jump into SASS yet, it’d be a learning opportunity. As far as implementation goes, I’d be curious to compare the potential for technical debt created by @grappler’s original idea with that of @tiagonoronha’s Grunt idea used in Storefront. Although, I’m not sure how to go about digging up that information. |
I would favor keeping SASS. A non-SASS user can download the theme from underscores.me with only CSS, but a SASS user would have to pick apart the stylesheet every time they created a new theme if scss files were removed (I use SASS in all my _s-based themes). SASS may also help encourage theme developers to minify all their CSS files, because no additional minify tools would be needed, only Because _s currently only has one stylesheet, it would be easiest to implement using just NPM scripts, although Grunt does have the advantage of supporting |
I'm wondering if introducing an additional build tool might be too opinionated, since, as @msroberts says, a user can compile Sass without additional tooling. I also use Grunt, but a lot of my colleagues prefer Gulp or Webpack. I wonder if it would a good idea (without producing too much overhead for maintenance) to add an example Gruntfile and an example Gulpfile? That would assist users new to the tooling in the discovery/education process, and those experienced with these tools can easily discard them in favor of their own configurations. |
I am not sure what you fully mean with technical debt in this case. What I can add is that most Grunt Tasks are based on NPM packages. It is proven that NPM packages are faster than Grunt or Gulp but I don't think this is a big factor here. As for some of the features in Storefront, I don't think we need a watch task as we are not actively developing the CSS. We have not minified the CSS and JS files in the past so we can leave the decision to the theme developer. @msroberts NPM has packages too for generating POT files and RTLCSS so there is no need for grunt tasks. @David-Brown I don't think having an example Gruntfile and Gulpfile is the solution here. Though it should not stop people from sharing their config files. |
This has been a great discussion, and I appreciate everyone being open to and brining different ideas. To double back to the question from @joshmcrty about data:
Since August, 2014 when Sass first dropped in _s and we began tracking it, about 13 percent of total downloads from underscores.me have included Sass. It has had its peaks and valleys but has stayed consistent with that number over time. Discussing whether or not Sass makes sense for _s on the whole is a good discussion to have at this point. I'm going to leave more questions here than answers while I continue to ponder this. 😄 I think it's hard to judge the implementation of Sass in _s purely as useful or not because the theme doesn't have any design. So the chances to take advantage of Sass' power are slim. It's better to judge it like we do _s on the whole: does it save you time or does it get in the way of doing your standard setup? The other question worth asking: If Sass were removed, could we do anything simple to make it easy to drop in Sass, Less or whatever else you want to do preprocessor wise? That's also an option. Right now, I'm leaning toward removing since it does seem like an opinion we should leave it up designers and developers. That said, I think we could still explore automating some of our other pain points, like the RTL styles and the POT file. I'd also say that this isn't about _s not being opinionated. I think it should get more opinionated, but not in the way of benefits for themers, but in the way of benefits that show up in the themes created from _s for end users. |
What I love about having SASS in If you remove SASS, it will be a step backwards, not forward. |
Don't forget Yarn - the new kid on the block |
I chatted with a handful of my colleagues on the Automattic Theme Team about moving this issue forward, plus have been thinking over all the comments here from the community... Here are some decisions: 🚀
cc: @grappler @m-e-h @jrfnl since you all have been heavily involved in build process discussions and PRs. |
Also worth noting that if there is a way to keep the |
@davidakennedy This seems to be the right decision. We currently have a few moving parts so will need to see in which order we do things. Here are a list of items that we need to keep track of.
|
@m-e-h I am getting |
Understrap.com adds _s and Bootstrap while using a Grunt/SASS/npm build process... |
We have previously discussed build tools for the CSS from Sass. In 2014 we decided against as we wanted to keep
_s
tool agnostic. #585, #1097, #1002, #661As it has been 2.5 years since that discussion I would like to propose a different solution.
Before merging pull requests related to CSS we need to be able to test that the Sass to CSS conversion is working properly. To be able to this properly every maintainer would need to have create a build tasks locally. Which is related to a previous discussion #615
What I propose we document the following command which does that without needing grunt or gulp or any other build system.
node-sass --include-path scss sass/style.scss style.css --output-style=expanded --indent-type=tab --indent-width=1
The only requirements for this is
node
andnpm install node-sass
which most developers are already using.After running this command I got the following changes. I have removed some of the code style changes that have not been factored in. I think adding another check to travis for CSS code styling is another issue that we should look at in another issue. Partially mentioned in #1146 and previously discussed in #936
The text was updated successfully, but these errors were encountered: