-
Notifications
You must be signed in to change notification settings - Fork 35
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
Comments
I agree that whatever namespace is set should apply to custom properties. @simonsmith, do confirm that SUIT CSS would like that? |
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. |
Yeah I don't see why we shouldn't allow something like that. |
Sounds good to me as well. |
Is the idea here to allow it, or to enforce it? That is, if you have defined the namespace |
maybe optional for now, forced in the next major release? |
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 |
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. |
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! |
Yes, that was my meaning as well
|
Cool, thanks for piping up. Open to a PR to speed things along. |
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? |
@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). |
@VinSpee : The decision whether to apply the same treatment to utility classes will affect what I'd consider the "correct" approach here. |
I would agree that they should apply to utility classes as well |
In my opinion there is no need to add namespace support to utility classes for three reasons:
|
Example: I have a spacing utility as a part of my base framework. It takes variables. I currently would like to have it as
It would also contain variables as Do ou think this would be better served as a component in this instance? How would that look? |
@VinSpee because of the single purpose nature of utilities you won't (or shouldn't) ever have duplicates. :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);
} |
@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 |
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.
@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 This discussion did come up some time ago on the SUIT repo and I agree with Nicolas' comment at the time:
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 :) |
Ok, well I'm fine with just namespacing custom properties. |
Sounds good! On that note, what do you think is the "right" way to implement here? |
I'm going to defer to @simonsmith and @giuseppeg on namespacing utilities.
If that is part of the argument, I think at the very least we should make it clear in the docs that the If that is the case, if someone really wants non-conflicting utils, might they do something like: |
@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. |
I think this is the way to add the feature:
|
Sorry I think the above may have been incoherent. I guess we need a custom property validating function. I'll look into it now. |
I'd love to be able to namespace custom properties, ala:
Is there any way that I'm missing to do this currently?
The text was updated successfully, but these errors were encountered: