peterramsing / lost

LostGrid is a powerful grid system built in PostCSS that works with any preprocessor and even vanilla CSS.
http://lostgrid.org
MIT License
4.5k stars 160 forks source link

Switch to another naming convention for variables #376

Closed equinusocio closed 7 years ago

equinusocio commented 7 years ago

Is this a feature request or a bug report? Yes

What is the current behavior? Variables follow the SASS convention

If it's a bug please provide the steps to reproduce it and maybe some code samples. Check http://lostgrid.org/docs.html#variables.

What is the desired behavior? I think variables should follow another convention rather than SASS. I can suggest to use the native css syntax like var(--lost-gutter)

What's the motivation or use-case behind changing the behavior?

Anything else? Yes, i love this project

peterramsing commented 7 years ago

For some context, here is some previous discussion on this: https://github.com/peterramsing/lost/pull/346#issuecomment-262831991

peterramsing commented 7 years ago

I'm totally fine to re-opening up the discussion, though. @codebysubtract, do you have any thoughts on this?

equinusocio commented 7 years ago

There is a problem using native css custom-properties? Plain css variables are just a suggestion. Why not a custom prefix? Something like #lost-gutter.

steve-holland commented 7 years ago

I'm not totally sure I'm seeing the need, I feel like it might be change for the sake of change. I think if there was a definite format we should be using for PostCss plugins and it'd just been missed, then that'd be a fair enough reason for changing it. What makes me a bit wary is, we've already had two possible candidates in this conversation for what to change it to, so it's not feeling like there is a "right answer" to what it should be.

If you went down the route of using the native css format, then I'm sure someone could argue you might get conflicts there, in the same way you could argue it might conflict with SASS as it currently stands. Then, if you come up with some new random format instead, who's to say a new shiny must-have plugin doesn't appear next week that uses the same format as we've selected and we're back to talking about more possible conflicts.

I do use LostGrid with SASS and I can't honestly say it's ever bothered me it was the same format.

@equinusocio Have you had problems with the current format or do you just not like it?

equinusocio commented 7 years ago

What about this? https://github.com/peterramsing/lost/pull/346#issuecomment-277817514

Since Lost is not SASS, using the sass convention can be confusing. I have no problem atm because my stack is full postcss based (lost, cssnext, cssnano – and sometimes precss(that use sass variables) ).

I think lost works well with sass if it's called before the sass process. Right?

steve-holland commented 7 years ago

@equinusocio - I see now where you're coming from. If you do call SASS first then Lost, it does break as an undefined variable in SASS, so that'll be a bad thing. So you've got me coming around to your way of thinking.

However, I would also agree with what @peterramsing has said on #346 in the future proofing section, that using the css variable naming convention might not be a good idea. I'd also say personally I'm not in favour of #lost-gutter either as at first glance it looks like a standard CSS ID selector.

I'll have a look around at some other PostCSS plugins and see if there's any standard prefixes which seem to get used.

equinusocio commented 7 years ago

What do you mean with "where you're coming from"? :)

We can use any of available symbols that are not used by sass, less, stylus: ~lost-gutter £lost-gutter ˆlost-gutter §lost-gutter ?lost-gutter

Or combine them: $~lost-gutter ..and so on.

PS: What do you think about to force users to put Lost before any other plugins that manipulate the css declarations?

steve-holland commented 7 years ago

@equinusocio - Sorry, that didn't translate very well, it's an English term, just means "I see your point of view".

This might be a stupid idea, but I'm just wondering if there's an alternative approach. What about it not being a variable, but a function call instead? I don't know how feasible something like this might be:

margin: lost-vars('gutter');

It would remove the need for picking a prefix. I'd need to look into if this is even possible, so it might be a terrible idea!

If it were possible though, it would also remove the need to force the order of plugins.

equinusocio commented 7 years ago

I like that! It feel more appropriate and let avoid all conflicts

steve-holland commented 7 years ago

@peterramsing - What do you think, is this a terrible idea?

peterramsing commented 7 years ago

@equinusocio LostGrid (being PostCSS) will need to come after any SCSS (Sass), LESS, Stylus, etc. PostCSS. However, I do think that we should do everything we can to avoid requiring LostGrid to be in a particular order for PostCSS plugins.

@codebysubtract I like margin: lost-vars('gutter');.

@equinusocio If I'm understanding, you think $lost-gutter will cause issues long term?

Like @codebysubtract mentioned, I think it's worth avoiding changes for change sake.

I do like @codebysubtract's idea of lost-vars('foo'). There could be reasons to move towards that as it might open up more uses for this that $lost-gutter offers.


This seems like a great conversation to keep going on but I'm not convinced that a change is needed yet and would prefer not change unless there is a need for a change. Less churn feels better.

equinusocio commented 7 years ago

Why should Lost use a sass naming convention for variables, making inconstency (from oter lost-* features) and conflicts, when it can use a something more appropriate like lost-vars() that remove all issues with plugins order?.

I see inconsinstency between lost-align, lost-center, lost-column etc.. and $lost-gutter. Yes we can simply force users to put Lost after all plugins that manipulate the css..but the cause should not be this inconsistency. My 2 cents.

Sorry for my english or if it seem aggressive, it's not my language :)

peterramsing commented 7 years ago

@equinusocio I appreciate your input! Thanks for sharing!

I'm still failing to identify where the existing naming convention would break based on the order...unless you were to use the exact same name in SCSS...which is valid I suppose.

lost-vars('foo') is a solid direction but if that's what we change to I'd want to make sure that it's the last change.

Regarding the inconsistency you mentioned, I see lost-center, etc to be vastly different than $lost-gutter. lost-center outputs a chunk of CSS as $lost-gutter outputs a value that corresponds to the global settings and $lost-gutter-local is associated with a particular lost-foo property.


I'll go back to my previous statement: if there doesn't seem to be a very strong reason to change then sticking to what is current I think is best.

So, the question is: Does matching the SCSS variable style cause concern that there will be undesirable naming conflicts?

peterramsing commented 7 years ago

If I were to answer that question I'd say no. If there is a conflict then SCSS will transpile out the value of the variable (which would be the expected outcome of an SCSS user). LostGrid will see a value there instead of the variable and there will be no issue.

That is how I feel. I'm open to my experience to not match others concerns.

equinusocio commented 7 years ago

@peterramsing Check https://github.com/peterramsing/lost/issues/376#issuecomment-308947553 and https://github.com/peterramsing/lost/issues/376#issuecomment-308951497. Now you can use Lost only if its called before all plugins that manipulate SCSS like syntax, like postcss-precss, postcss-each, sass and many more otherwise other plugins will return an Undefined variable error. So, now users are forced to put Lost before all other plugins..if this is good for you, i will accept that, but in my opinion this is an issue of inconsistency that is easily (i think) to fix.

Lost should be incapsulated and agnostic (since it can be used in many environments) and it should use a "private" naming convention. :)

steve-holland commented 7 years ago

@peterramsing I did do a quick test on my setup which has SASS called first, then PostCSS. @equinusocio is right, you do get an Undefined variable error in SASS as it thinks $lost-gutter is a SASS variable not a PostCSS one.

equinusocio commented 7 years ago

@codebysubtract yes, and you will get the same error also not using SASS but other postcss plugins that parse a sass like syntax like postcss-precss, postcss-each, postcss-simple-vars, sass and many more..

peterramsing commented 7 years ago

@codebysubtract Thanks for testing that!

That's certainly reason enough to change that! I scrolled back through and misread (and didn't fully ponder it with a cup of coffee). I kept thinking "yeah, it'll do weird things if you define the var in SCSS and in LostGrid. ...but I didn't fully process if you just defined it for LostGrid in SCSS... that makes perfect sense!

Thanks for your patience with me.

do we like lost-vars('gutter')? is that the consensus?

equinusocio commented 7 years ago

Yes, for me lost-vars() is a nice solution altough i think it will be a breaking change.

peterramsing commented 7 years ago

It wouldn't have to be a breaking change unless we remove the old version–we could ship the new version in a point release as long as the old version stuck around–and then remove it in a point release eventually.

equinusocio commented 7 years ago

any update? will you consider this change?

steve-holland commented 7 years ago

@equinusocio - Yes, I believe we're going to be doing this change. If you notice this issue has been tagged with 'backlog'. So as far as I'm aware that means it's on the 'todo' list.

peterramsing commented 7 years ago

Yup, I put it in the backlog. 😇 I suppose this could be a "bug" if it throws errors in SCSS transpilation. 🤔 Let's finalize the spec for this and we can get it slated for dev.

equinusocio commented 7 years ago

Oh ok. thank you :)

peterramsing commented 7 years ago

So we've decided we like lost-vars('foo')? I kind of want to play around with that some more–see how it scales, how it'd be defined, etc. It's the lost-gutter-local that I think might be a bit odd here–but maybe not. Would want to test it.

Regarding breaking change or not. I think there isn't any reason to just bump to 9.0. Why not? I think in an ideal world we'd keep the old variables around with deprecation warnings. That sounds like the right thing to do and I even have a note to write up some "Here's how LostGrid drops features..."

equinusocio commented 7 years ago

I agree. You think about return also a console log warning?

peterramsing commented 7 years ago

@equinusocio Yeah.

peterramsing commented 7 years ago

Closed by #389

equinusocio commented 7 years ago

Awesome guys! :)