Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
289 stars 76 forks source link

It is unclear how to apply a custom theme #507

Closed mjuniper closed 4 years ago

mjuniper commented 4 years ago

We would like to begin using these components (which I think are really great) in Hub. But we allow users to define custom themes which we would need to apply to calcite-components at runtime.

I understand how to do this: Set the appropriate css custom properties as described here.

But it is not clear exactly which custom props need to be set.

I took a stab at this for calcite-button which I thought would be pretty easy. I started out by just trying to set some of the props defined here but that seemed insufficient and I ended up having to set --calcite-ui-blue-1 too. I landed on something that mostly worked except for the hover styles. But it was sorta painful to get there. Which makes me wonder if I am missing something.

Is there further documentation on this somewhere? Happy to help if I can.

macandcheese commented 4 years ago

Good issue! At this time we do not have a great story around custom theming, although it's something we are actively working on.

As you noted there are a lot of cases where the underlying colors have been re-aliased inside components - 'calcite-button' actually being one of the hardest to override due to this, which would need to be rectified before a global theme override could propagate to each component (many were built before we had our "calcite-ui-*" css vars)

There are a few discussion points we'd like input on for this...

Do you just foresee needing to provide 3 "brand color variants" (blue-1, blue-2, blue-3 in our case)?, or do you want to have facilities for overriding all interface status colors (green for success, red for error, etc.).

To dive into that more, if the "brand color override" provided by a user is green, how does that work with a "success" state that's also green, if it's red how does that work with distinction for "error" states, etc.

More insight there could help us figure out a better story for allowing some kind of overrides for theme.

Two other things to consider are "light/dark" versions of a theme (you can provide overrides for both with css vars), and accessibility - currently we are able to ensure accessible components with our blue-1/2/3, does Hub "theming" UI have considerations for this?

Apologies for the wall of text but hopefully these questions can give us a little insight into how to make theming a good experience!

julio8a commented 4 years ago

@ffaubry, SInce ArcGIS Dashboards has similar theming requirements, can you give some input on this too (if applicable)?

bpatterson88 commented 4 years ago

Some Dashboards requirements/requests related to theming ...

@ffaubry can add additional color if necessary.

paulcpederson commented 4 years ago

Yeah I think if all components point to our internal css vars rather than re-aliasing them we could accomplish what dashboards need.

FelFly commented 4 years ago

@macandcheese I might not have fully understood your question, but I'm going to try to explain our model. As far as Hub Theme's goes, we have two modes. Edit Mode, which is Hub's Builder and which uses the Calcite (Light) theme always, and Live Mode, which is the output of the builder and uses Calcite Light is nothing else has been configured. That is, we import the Shared Theme from ArcGIS Online if it's present during initialization and if it's not, then we load our customer theme color pickers with hex values from the Calcite Light theme. However, our customers have the option to change those.

We then take those values, which are presently:

*being unique to Hub and not available in the Shared Theme

and use expand them to set the other ~44 other color variables controlling the theme in the app, which includes luminance functions that darken or lighten values based on background color to improve accessibility contrast. (eg. We currently have a checkbox component that has a white checkbox box. If the customer is using black as their body background and light pink as their link color, we might use the light pink as the checkmark, but we darken it because we've detected it shows on a light background.)

We do try to keep some types of components standardized between both modes, like alerts, tooltip text, and dropdown menus. We do not use customer theme variables in these cases, but in the majority of the live view (checkboxes, tabs, radios, buttons, modals, inputs, etc) we are going to attempt to use the values the customer set in a way that assumes they picked legible colors for things like body-background vs body-link.

Lastly, we do not provide customers the option to toggle on a dark theme or set dark theme values at this time.

mjuniper commented 4 years ago

Expanding on @FelFly's comments:

I believe we would also need font-family and perhaps font-size.

Do you just foresee needing to provide 3 "brand color variants" (blue-1, blue-2, blue-3 in our case)?, or do you want to have facilities for overriding all interface status colors (green for success, red for error, etc.).

I think we would not need the status colors but it would be nice.

Two other things to consider are "light/dark" versions of a theme (you can provide overrides for both with css vars), and accessibility - currently we are able to ensure accessible components with our blue-1/2/3, does Hub "theming" UI have considerations for this?

We definitely do not need dark/light theme. As @FelFly said, we do run the theme through some code and end up with ~44 css variables that we are ultimately setting. This is primarily to account for accessibility/contrast.

From my perspective, the ideal scenario would be for me to set CSS custom props that correspond to the organization's shared theme (and also font-family and maybe font-size) and have everything just work.

A fallback that I'd be pretty happy with is that it would be on the consumer to "expand" the shared theme into a somewhat larger number of props (ensuring appropriate contrast, etc) and set those. Maybe these + font-family and maybe font-size?

At a minimum what I'd like to see is clear documentation about what css custom properties are exposed for each component.

Please let me know your thoughts and what our team can do to help.

paulcpederson commented 4 years ago

I think the only place we have a declaration for font-family is in the global style sheet. All the components themselves use font-family: inherit. This means that you could simply set font-family to whatever you want via css and all the components will automatically inherit your new selection:

Screen Shot 2020-05-01 at 1 18 47 PM

The direction I think we're heading in is to make all of our colors pull from the colors set in global.css as well. Previously we experimented with surfacing different color vars for different components (example: --calcite-loader-spot) but we've been moving everything to use the standard calcite names (ie. --calcite-ui-blue-1). So as a dev all you'd have to do at that point is something like:

.my-cool-theme {
    font-family: "Comic Sans"
    --calcite-ui-blue-1: red;
    --calcite-ui-blue-2: orange;
    --calcite-ui-blue-3: purple;
    ...all other vars
}

Anything you didn't override would inherit the defaults.

julio8a commented 4 years ago

Yes, that's the direction we're going. A while back I spend a few cycles going through component styles and changing whatever aliased CSS variables they were using and switching them to global CSS vars from here: https://github.com/Esri/calcite-colors/blob/master/colors.scss#L531

mjuniper commented 4 years ago

Ha, yes, just setting font-family works. Shoulda tried that!

The direction I think we're heading in is to make all of our colors pull from the colors set in global.css as well.

Do you mean that should work now? When I tried that for calcite-button that seemed insufficient. But it looks like that will work for calcite-card.

macandcheese commented 4 years ago

Do you mean that should work now? When I tried that for calcite-button that seemed insufficient. But it looks like that will work for calcite-card.

It will work for some components currently - in others (like button) - we had "re-aliased" CSS Vars to use component-specific nomenclature so they won't work as expected. We'll be going through all the component files shortly to remove those references and then add some demos showing how a single set of overrides should propagate to all components.

mjuniper commented 4 years ago

Thanks guys. I took a stab at simply setting calcite-colors custom properties and that seems to be working out very well. It even mostly works well for calcite-button.

I think there is not really a good mapping that I can apply wholesale from site theme => calcite-colors without messing up other components since site theme specifically defines buttonBackground and buttonText. So some component specific overrides seem to mostly take care of that:

calcite-button[disabled] {
  cursor: not-allowed;
}
calcite-button {
  --calcite-button-blue-solid-color: $button-text;
}
calcite-button[appearance=clear] {
  --calcite-blue-accessible: $body-link;
}

I wonder if that disabled state rule should be incorporated into calcite-components?

macandcheese commented 4 years ago

@mjuniper - thanks, that's helpful context. So, $button-text and $body-link are custom values generated by a user while creating their theme here?

Once we get to a "cleaned up" state - those two bespoke values (calcite-button-blue-solid-color and calcite-blue-accessible) will be replaced with values that come directly from calcite-colors (calcite-ui-blue/1/2/3).

However, you're correct in that you could more explicitly define overrides per component vs. just wrapping an entire project and having the custom values propagate to children, so depending on what colors you have to work with from your custom theme, maybe doing it per component like this makes sense for that use case (or, just in a few cases for certain components...)

Regarding cursor: not-allowed - I don't have a strong opinion about adding that to button or other components, we generally reduce opacity to 0.4 and add a pointer-event: none rule for disabled items. We could add it in by default, or let folks add if they desire like you did - we can see if @Esri/calcite-components has any thoughts there.

mjuniper commented 4 years ago

So, $button-text and $body-link are custom values generated by a user while creating their theme here?

Yes, exactly.

I've run into something else now. I just realized that my hub branch is pointing at 1.0.0-beta.22. But when I upgraded it to point to 1.0.0-beta.25, my theming stopped working.

It looks like it has something to do with a change to how you're scoping calcite-colors variables. I believe previously you were scoping them to :root so I could just define my default theme styles like this:

:root {
  --calcite-ui-blue-1: red;
}

And because my stylesheet comes later my colors win.

But it looks like you are now scoping them to [theme=light]. So that's fine, I can just scope my default theme like that too. But I need to set a custom theme which I don't have until runtime so I am setting the CSS custom properties with javascript and it is not obvious to me how to scope it properly so my colors win.

Previously (when your variables were scoped to :host), I could just do something like document.documentElement.style.setProperty('--calcite-ui-blue-1', 'fuchsia') but that doesn't work now. The only thing I have found that works is something like document.querySelector('calcite-button').style.setProperty('--calcite-ui-blue-1', 'green') but that's not really viable because I want to apply the theme to the whole document, not element by element.

Any suggestions are appreciated, thanks!

mjuniper commented 4 years ago

This is a working example pointing at beta-22: https://codepen.io/mjuniper/pen/yLYpwmM

This is a non-working example pointing to beta-25: https://codepen.io/mjuniper/pen/abvExoG

mjuniper commented 4 years ago

Ok, I see what is different at beta-25: components with no theme specified get a theme="light" attribute. And that is causing the [theme=light] {...} rules to win over :root {} rules.

But that leaves me wondering: how can I set the css custom properties so that I can set, for instance, calcite-ui-blue-1 for the entire page? Again, this was working prior to beta-25.

I am doing things like this:

document.querySelector(':root').style.setProperty('--calcite-ui-blue-1', 'fuchsia');

But since components now always have the theme attribute, the [theme=light] rules from calcite colors win even after I do that.

paulcpederson commented 4 years ago

@mjuniper you can just make your selector more specific than the theme attribute selector on the element. So something like:

body * {
  --calcite-ui-blue-1: fuchsia
}
mjuniper commented 4 years ago

Thanks for the suggestion @paulcpederson.

Remember I don't have my colors until runtime. So I can set the defaults the way you suggest but I don't think I can do something like that to change the values dynamically (in the browser, at runtime) because if I do this:

document.querySelector('body *').style.setProperty(...);

I will only have the first element that matches the selector. I guess I could use querySelectorAll and loop through all the elements:

document.querySelectorAll('.hydrated').forEach(el => {
    el.style.setProperty('--calcite-ui-blue-1', 'fuchsia');
  });

But that sure isn't as nice as:

document.querySelector(':root').style.setProperty('--calcite-ui-blue-1', 'fuchsia');

And remember also that I am not just setting one property; I am looping through the keys of an object and setting a prop for each key. If I have to loop through all the calcite components on the page for each key, that makes me start worrying about perf.

Also it just hit me that I think this won't work anyway. Imagine this scenario: I land at the page and apply the theme by looping through all the calcite-components on the page and assigning all the custom props for all of them. Then the user interacts with the page in such a way that a new component is rendered. ~I'm pretty sure~ It would not be themed.

And also note that querySelectorAll('[theme=light') is the best way I could think of to select all the calcite-components on the page but yuck.

I am fairly new to css custom properties so maybe I am missing something fundamental. Thanks for your help.

mjuniper commented 4 years ago

I just noticed also that this does not affect all calcite-components. Some have the theme attribute defined thus:

@Prop({ mutable: true, reflect: true }) theme: "light" | "dark";

I believe they will not get a theme attribute if the consumer does not assign one. But some have the theme attribute defined thus:

@Prop({ reflect: true, mutable: true }) theme: "light" | "dark" = "light";

I believe they will get a theme attribute if the consumer does not assign one. And they will exhibit this issue.

paulcpederson commented 4 years ago

This is the problem we were trying to solve with host-context a while back (that led to its own problems...). I agree we should not set theme by default, but I seem to recall it led to some issues, maybe IE specific? I don't fully remember, maybe @macandcheese recalls...

macandcheese commented 4 years ago

Yeah we went through a bunch of iterations of trying to solve theme issues in IE...

Some of which were caused by setting vars directly on :root, others that had to do with nested children not correctly inheriting vars set on parents....

:host-context worked in IE but not Safari / Firefox... We can try to go through and remove the "fallback" theme prop and confirm that we still don't have issues.

I'm not sure if you offer a "live preview" of the themed components to users, currently you'd need to trigger re-renders of the components, we had looked at a couple options Stencil provides for updating but none were particularly attractive...

mjuniper commented 4 years ago

Well IE is a whole 'nother issue altogether. I have site theming (at runtime, in the browser) via CSS custom properties working for native elements in ie using this library. But I have not been able to get the props to flow through to calcite-components in ie at all.

ajturner commented 4 years ago

@paulcpederson @macandcheese You've mentioned Stencil-store as a way to set theme across all components. Would it be viable to use this with a configuration component that allows an instance to set a lot of variables that are then used across all components?

for example:

<calcite-theme
    calcite-ui-blue-1="red"
    calcite-ui-red-2="#0f42ef"
></calcite-theme>

This component would set global state variables and/or set CSS variables that are used within each componentWillLoad or similar?

The theme could even be named and referenced explicitly later if several themes need to be supported:

<calcite-theme
    theme-name="red"
    calcite-ui-blue-1="#ff0000"
></calcite-theme>

<calcite-theme
    theme-name="blue"
    calcite-ui-blue-1="#0000ff"
></calcite-theme>

<calcite-button
    theme="red"
>I'm a red button</calcite-theme>

<calcite-button
    theme="blue"
>I'm a blue button</calcite-theme>
ajturner commented 4 years ago

unless I'm fundamentally misunderstanding what Stencil-store does and if it allows for cross-instance data sharing

macandcheese commented 4 years ago

Yes I believe that's the idea with stencil store and Statetunnel - both of which should (?) allow for live updates to components w/out re-render. Ideally the "Calcite-Config" component could also set light / dark (if needed), global scale, etc in addition to being a theme provider - all these values could then more explicitly be overridden at the component level if needed.

I'm not sure about the theme=red/blue on the actual Calcite-buttons, as the component and others would probably just accept overrides for the Css vars used by the existing "color=Red/blue/light/dark" prop...

paulcpederson commented 4 years ago

@mjuniper so I investigated this a bit today. If you check out the paulcpederson/theme branch locally you can play around with it. Basically I've removed all instances of setting a theme if none is provided. I've also made quite a few updates to move from custom variables per component to using the global theme variables, so theming should work more consistently across more components.

Here is a screenshot where I've turned the entire UI into a 100% purple mess via only setting css vars on root:

Screen Shot 2020-05-07 at 6 02 26 PM

Let me know if this would solve your use case and we can try to get this into the next release!

mjuniper commented 4 years ago

@paulcpederson I pulled that branch down and played around for a few minutes - yes I think that would work great! Thank you.

macandcheese commented 4 years ago

This was in the last release - going to close this issue, @mjuniper @ajturner @FelFly feel free to open another issue if anything else comes up. Thanks for the discussion all and the fix @paulcpederson !

paulcpederson commented 4 years ago

I have a PR open that adds a summary of all of this to our conventions document as well: https://github.com/Esri/calcite-components/pull/613