Replies: 8 comments 2 replies
-
additional thoughts on some of the points in these scenarios:
This is an interesting one on how we have implemented the CSS API. Atm, I have noticed this isn't standardised as much as it should be even though we have tried to, so it needs looking at on a per-component basis once we figure out what solution to take. It differs even in Button itself, where we use However, when it then comes to state based classes/pseudoclasses such as One solution to needing the user to tie both to classname and CSS variable is providing e.g.,
To emphasise, this will require work from design to extend the theme and provide e.g. a border-radius foundation (which I think is in the pipeline eventually) as well as a concrete decision on the theme being in a stable place - so this would potentially require more thought and dedicated work than approach a
General note: makes breaking changes through classname/DOM structure more likely.
Imo, I think the approach for this scenario is fine and is what we can move forward with. It's almost like adding a new feature to the design, rather than changing an existing style of our component design (color, border width etc) alon.
Another interesting one, do we allow (encourage) for our local tokens Plus, if they are extending variant, potentially could cause issues depending on what loads in first, i.e. will they need to be more specific on the default variant, making this actually
This one will depend on how well we document the theme. Will be important for users to understand what components use actionable/what the actionable characteristic affects (documenting both ways <->, i.e., in storybook we currently list in the component pages the characteristics used, but the characteristics pages ideally should list what components consume it). This education may let the user tie together their customisations (e.g., user wants to change color of ToggleButton and secondary variant Button etc - realising actually changing theme like approach a would be better in this use case, but if they wanted to change only secondary variant Button and ToggleButton not being affected, approach b is better).
See above comment on approach a
Agreed. Ideally need designers to look on per-component basis on what can be customised which will require potentially a lot of research/effort. cc @bhoppers2008
I don't think |
Beta Was this translation helpful? Give feedback.
-
Notes from current POC implementation, which will benefit and probably change from this conversations. Scenario 1For this scenario , we are currently overriding at a theme level (option b).
and adding it to the component
having this as a variable would be useful but it doesn't represent a lot of benefit as it would mean a lot of variables would need to be declared. Scenario 2For this one, I think it would be beneficial to think on how we use CSS shorthands. In the case of borders, where we declare a border
splitting this to having |
Beta Was this translation helpful? Give feedback.
-
following @Fercas123 comment
This is quite a unique one where it's the local token
We would have a lint rule in Salt for this to convert it to -borderRadius (CSS attributes always camel case). I wonder if we should document the rules (doc'd in confluence) for consumers when creating their tokens, so that they appear more similar to ours, which could make code look cleaner overall?
I think this is ok for now in what we don't have in the theme, and when we eventually have border radius foundation (or e.g. ..--salt-actionable-borderRadius characteristic), users can replace this with the tokens we provide
do you mean having a border radius API token on the component or as you've done above?
that's interesting too - I think it depends on how strict we want to be with the design being the border at the bottom. I see this as one that highlights why it's important for design to look on a per-component basis and decide whether we would want to go this way as is, or to provide the border width token for users to place it in different positions.
|
Beta Was this translation helpful? Give feedback.
-
Thanks for putting this together My view is that if we can allow theming/component customisation primarily through css variables, this in theory provides the most flexibility and the easiest change management. However the caveat is that to do this effectively requires many more css variables which would be unmanageable without investment in tooling. With regards to the proposed scenarios:
Prefer Approach B, but there is an additional approach which is we could provide an additional variable in our theme: --salt-button-cta-background: var(--salt-actionable-cta-background); This would allow consumers to target cta buttons without affecting other actionable components, if this is something we wanted to support for ultimate flexibility.
Could be simplified with an additional variable e.g. we provide an extra variable such as: --salt-accordion-border: var(--salt-size-border) var(--salt-separable-borderStyle) var(--salt-separable-tertiary-borderColor); Then consumers could use: .customAccordion.saltAccordion {
border-bottom: var(--salt-accordion-border);
} Again this would increase the amount of variables.
Probably prefer Approach B which feels more inline to how we add variants ourselves, and I see consumer variants as extending our theme/styling
Worth mentioning that with the approach of only including overrides for theme variables, we can still support things like border-radius on button, but it means taking a view that |
Beta Was this translation helpful? Give feedback.
-
some thoughts in reply to @el-dav :)
I (and I can probably speak for design) would be against tying components to the theme. It's good to have the theme as a standalone that could be used even for another set of components thats not Salt, a product in its own right. Though, this is basically the local token
or directly defining the background in each class itself and getting rid of the local token
it would increase API tokens, but decrease local...
agreed - and I think that is designs POV too (see above comments) |
Beta Was this translation helpful? Give feedback.
-
Approach A - I'd recommend this If an additional variant or validation status is needed across multiple components
|
Beta Was this translation helpful? Give feedback.
-
you're right, I confused myself because we treat variants and state slightly differently - I guess what I meant to say is the bit below that comment I made (the thought it eluded to 😄 ) |
Beta Was this translation helpful? Give feedback.
-
this was a maintainers discussion, closing due to inactivity |
Beta Was this translation helpful? Give feedback.
-
To customize Apps at the theme level, there are different ways to achieve through code. We want to gather the how as well as pros and cons.
Scenario 1: I want all my CTA Buttons to be in purple (in both theme/modes)
Approach a: with
--saltButton-background
Although
--saltButton-background
is defined to be used for.saltButton
, we can't just provide the color to the theme otherwise all variants (CTA/primary/secondary) would be impacted. So there is a cross-reference shared knowledge between the theme and the component structure.This approach means the user's code is tied to both classnames (
.saltButton-cta
) as well as CSS variable API (--saltButton-background
)Approach b: with
--salt-actionable-cta-background
This follows how Salt defines theme, where characteristics (actionable in this case) are defined separately than components. However this does mean our consumer needs to fully understand the the structure of the theming system.
This relies on characteristic variable (
--salt-actionable-cta-background
) alone, which means our components needs to be designed to have such variable exposed/consumed, e.g., button border radius is not at the moment.Approach c: wrap with custom class
Offer a
CustomButton
by wrapping Salt Button, so it's isolated but no longer tie to the theme anymore.Scenario 2: I want additional border at the bottom of my Accordion
Current Accordion only supports top border, so adding a new bottom border can only be done through wrapping with new class name.
Salt won't be offering every properties to be overridable, so addressing unknown requirement purely in theme won't be feasible for all use cases.
Scenario 3: I want additional variants for Button
Additional variants (e.g. foo, bar) on top of CTA, primary and secondary would be only possible through wrapping a component. However, whether how much of the customization should be component specific or at the theme level is a case-by-case consideration.
Approach a: create new actionable characteristics variables and use in Button with new class name
Given foo/bar actionable is an extension, prefix are chosen to be not using
--salt
and it can be scoped without.saltTheme
to work. Also note that the use of--button-background
v.s.--saltButton-background
, where it's likely the result if our user is copying code and structure from Button.css.Approach b: add button specific overrides
Although Salt button variants are tied to actionable characteristics, that's not necessarily the case for an App needs override.
Potentially
--button-background
would also work, or even justbackground
. Which one to recommend depends on our recommendation.Scenario 4: I want border radius for Button
Unlike above scenario, although border radius is not part of theme, Salt Button has
--saltButton-borderRadius
API available which defaults to 0.From recent discussion, the choice of which property to have component CSS variable API feels a bit random. We should make it more formal, e.g., one proposal is that only tokens from the theme should have corresponding CSS API.
From above code, both class name (
.saltButton
) and component CSS variable API (--saltButton-borderRadius
) is used, so both need breaking change management.Summary
Discussions
--button-background
)?classes
API, so any class name change can be detected.border
property (short-hand) @Fercas123Beta Was this translation helpful? Give feedback.
All reactions