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

namespaced custom properties #79

Open
VinSpee opened this issue Feb 9, 2016 · 26 comments
Open

namespaced custom properties #79

VinSpee opened this issue Feb 9, 2016 · 26 comments

Comments

@VinSpee
Copy link

VinSpee commented Feb 9, 2016

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
Copy link
Contributor

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

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

@simonsmith
Copy link
Collaborator

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
Copy link

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

@timkelty
Copy link

timkelty commented Feb 9, 2016

Sounds good to me as well.

@davidtheclark
Copy link
Contributor

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
Copy link
Author

VinSpee commented Feb 10, 2016

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

@giuseppeg
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Author

VinSpee commented Feb 10, 2016 via email

@davidtheclark
Copy link
Contributor

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

@VinSpee
Copy link
Author

VinSpee commented Feb 17, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

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

@VinSpee
Copy link
Author

VinSpee commented Feb 18, 2016

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

@giuseppeg
Copy link

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

  • u- is already a namespace
  • you can already add levels to the namespace, e.g. suicss/utils-size uses the u-sm- to namespace the size utilities for small devices
  • utility classes are single purpose and final i.e. they use !important and can't (shouldn't) be overridden. So for example I don't see why you would make .ns-u-posFixed when you already have .u-posFixed.

@VinSpee
Copy link
Author

VinSpee commented Feb 18, 2016

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
Copy link

@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
Copy link
Contributor

@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
Copy link
Collaborator

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
Copy link
Contributor

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

@VinSpee
Copy link
Author

VinSpee commented Feb 18, 2016

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

@timkelty
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

I think this is the way to add the feature:

  • Extend the library so that the componentName pattern can be a function, like componentSelectors: A single function that accepts a component name and returns a regular expression, e.g. js componentSelectors: function(componentName) { return new RegExp('\\.ns-' + componentName + '(?:-[a-zA-Z]+)?'); }. The pattern to follow here is component selectors.
  • Modify the suit pattern in preset-patterns.js to use that feature.

@davidtheclark
Copy link
Contributor

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

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

No branches or pull requests

5 participants