postcss / postcss-bem-linter

A BEM linter for postcss
MIT License
572 stars 35 forks source link

namespaced custom properties #79

Open VinSpee opened 8 years ago

VinSpee commented 8 years ago

I'd love to be able to namespace custom properties, ala:

/** @define Button */

:root {
  --ns-Button-size: 1rem;
}

Is there any way that I'm missing to do this currently?

davidtheclark commented 8 years ago

I agree that whatever namespace is set should apply to custom properties.

@simonsmith, do confirm that SUIT CSS would like that?

simonsmith commented 8 years ago

We don't currently have any formal guidelines for custom properties, although there is a long outstanding issue to get it all written up. I think it makes sense to allow components to have namespaced custom properties, and we can include that in the documentation when I get round to doing it.

cc @timkelty @giuseppeg

giuseppeg commented 8 years ago

Yeah I don't see why we shouldn't allow something like that.

timkelty commented 8 years ago

Sounds good to me as well.

davidtheclark commented 8 years ago

Is the idea here to allow it, or to enforce it? That is, if you have defined the namespace ns for your stylesheets, should we enforce that every custom property in a linted stylesheet has an ns prefix? Or should we add this as an option?

VinSpee commented 8 years ago

maybe optional for now, forced in the next major release?

giuseppeg commented 8 years ago

If it is enforced you won't have any choice but to release a major version.

maybe optional for now, forced in the next major release?

+1

In this iteration we should just add support for namespaces and make it fail if the component part is missing otherwise things like --ns-fooBar would be valid – but I guess that this was obvious.

simonsmith commented 8 years ago

If it is enforced you won't have any choice but to release a major version.

Nothing wrong with releasing major versions.

I'd vote that if you have a namespace option set then it should apply to the custom property too.

giuseppeg commented 8 years ago

I don't know what is their release cycle like, often times people prefer to have a milestone and include more than one branch before making a major release.

I don't have any problem with that anyway!

VinSpee commented 8 years ago

Yes, that was my meaning as well

davidtheclark commented 8 years ago

Cool, thanks for piping up. Open to a PR to speed things along.

VinSpee commented 8 years ago

I'm happy to give it a whirl. @davidtheclark I noticed that the only place we're checking for namespace now is in the component checker, which brings a different context than the custom property checker. How can I appropriately pass the namespace into the custom property checker? Any "correct" way that I'm missing?

davidtheclark commented 8 years ago

@simonsmith , @giuseppeg , @timkelty : Should namespaces also be applied to utility classes? I'd expect so, myself, but that's not what's going right now. Unless there's an objection, I'd think we should take this advantage to use the namespace everywhere (component classes, utility classes, custom properties).

davidtheclark commented 8 years ago

@VinSpee : The decision whether to apply the same treatment to utility classes will affect what I'd consider the "correct" approach here.

VinSpee commented 8 years ago

I would agree that they should apply to utility classes as well

giuseppeg commented 8 years ago

In my opinion there is no need to add namespace support to utility classes for three reasons:

VinSpee commented 8 years ago

Example: I have a spacing utility as a part of my base framework. It takes variables. I currently would like to have it as

.blank-u-mgBsm

It would also contain variables as --blank-sm

Do ou think this would be better served as a component in this instance? How would that look?

giuseppeg commented 8 years ago

@VinSpee because of the single purpose nature of utilities you won't (or shouldn't) ever have duplicates. This means that you can define .u-mgBsm and a generic variable --u-mgBsm

:root {
  --u-mgBsm: 1em;
}

.u-mgBsm {
  margin: var(--u-mgBsm);
}

and then in your theme/framework override the variable

:root {
  --u-mgBsm: var(--blank-sm);
}
davidtheclark commented 8 years ago

@giuseppeg , @VinSpee : What about utilities with more generic names that could be implemented in different ways? Let's say I import a library that has a u-clearfix class, but I don't like the implementation so want to create my own, nm-u-clearfix?

simonsmith commented 8 years ago

I'd be inclined to agree with @giuseppeg in that a utility should generally be unrelated to components. Configuring them with custom properties is rare, but a valid route to go down if they need it.

Let's say I import a library that has a u-clearfix class, but I don't like the implementation so want to create my own, nm-u-clearfix?

@davidtheclark That feels quite edge case. Utilities tend to have a very specific purpose and are often only a few declarations if that. I couldn't imagine there would be a radically different u-cf implementation.

This discussion did come up some time ago on the SUIT repo and I agree with Nicolas' comment at the time:

you should be using utilities for styles that can work in any app

So far I've only seen hypothetical reasons as to why you'd need to namespace them, rather than a real world example. Having used SUIT on a few projects I've still yet to bump into this issue.

Happy to be proven wrong though :)

davidtheclark commented 8 years ago

Ok, well I'm fine with just namespacing custom properties.

VinSpee commented 8 years ago

Sounds good! On that note, what do you think is the "right" way to implement here?

timkelty commented 8 years ago

I'm going to defer to @simonsmith and @giuseppeg on namespacing utilities. I could go either way.

u- is already a namespace

If that is part of the argument, I think at the very least we should make it clear in the docs that the u is occupying the namespace column.

If that is the case, if someone really wants non-conflicting utils, might they do something like: .myU-mgBsm? Then I assume we'd have to have the linter have a setting for utilityNamespaces or something that defaulted to ['u'].

davidtheclark commented 8 years ago

@VinSpee Sorry, I'm going to need some time to look at the code and think about it. If you come up with any proposals, feel free to PR. Otherwise I'll try to get to it this weekend.

davidtheclark commented 8 years ago

I think this is the way to add the feature:

davidtheclark commented 8 years ago

Sorry I think the above may have been incoherent. I guess we need a custom property validating function. I'll look into it now.