cerner / terra-core

Terra offers a set of configurable React components designed to help build scalable and modular application UIs. This UI library was created to solve real-world issues in projects we work on day to day.
http://terra-ui.com
Apache License 2.0
183 stars 165 forks source link

Create consumer button themes #729

Closed zhongyn closed 6 years ago

zhongyn commented 7 years ago

Issue Description

As an application developer, I need predefined consumer button themes available so that i can apply it to terra button.

Example:

import Button from 'terra-button';
import 'terra-button/consumer-button-themes.css';

<Button className='consumer-button-default-theme' />
<Button className='consumer-button-concept1-theme' />

Generating Theme CSS

Solution 1: Basic CSS

consumer-button-themes.css

.consumer-button-default-theme {
  --color: #123456
  --terra-button-active-background-color-default: rgb(--color, 0.2);
  --terra-button-active-border-color-default: rgb(--color, 0.5)
  --terra-button-active-color-default: --color;  
  ....(all 34 color variables)
}

.consumer-button-concept1-theme {
  --color: #adsqew
  --terra-button-active-background-color-default: rgb(--color, 0.2);
  --terra-button-active-border-color-default: rgb(--color, 0.5)
  --terra-button-active-color-default: --color;  
  ....
}
......

Obviously, there is a repeating pattern for generating theme classes.

Solution 2: SCSS

$color-map: (
  default: #123456,
  concept1: #qwerty,
  ...
);

@each $color in $color-map {
   .consumer-button-#{$color}-theme {
        --terra-button-active-background-color-default: rgb($color, 0.2);
        --terra-button-active-border-color-default: rgb($color, 0.5)
        --terra-button-active-color-default: $color;  
        ....
   }
}

Solution 3: PostCSS plugin Need to look into that, would be great if you have an idea.

Issue Type

mhemesath commented 7 years ago

Use solution 1... themes should be purely custom prop based and shouldn't have dependencies on SASS. Doing so would make applying the theme much more problematic as we'd have to include a SASS compiler.

mhemesath commented 7 years ago
.consumer-button-default-theme {
  --color: #123456
  --terra-button-active-background-color-default: rgb(--color, 0.2);
  --terra-button-active-border-color-default: rgb(--color, 0.5)
  --terra-button-active-color-default: --color;  
  ....(all 34 color variables)
}

No need to put each component in its own class name. Just have a single selector for each theme type.

zhongyn commented 7 years ago

@mhemesath By single selector do you mean this:

.consumer-button-theme {
  .default { --color: #123456; }
  .concept1 { --color: #qwe123; }

  --terra-button-active-background-color-default: rgb(--color, 0.2);
  --terra-button-active-border-color-default: rgb(--color, 0.5)
  --terra-button-active-color-default: --color;  
  ....(all 34 color variables)
}
mhemesath commented 7 years ago

almost.. don't scope concept themes under consumer themes (make them a separate theme).

alex-bezek commented 7 years ago

nit: I would assume you don't want the default class, just declare the css variable? I'm assuming this would have some issues if the consumer forgot to use the .default and then the variable wasn't declared, and there are no fallbacks where you are using var(--color) https://youtu.be/2an6-WVPuJU?t=1613 this video shows using just a --color: black; at the top, but talks about some specificity issue and then they use "default defaults".

EDIT: @mhemesath my understanding is the consumer theme dictates some default values and provides the logic on how to derive values of hover, active, etc states based off a single initial value. Then concept's are just a new initial value's that can use the same calculations. Are you saying to make each concept its own theme file?

zhongyn commented 7 years ago

@mhemesath @alex-bezek how about this:

.consumer-button-theme {
  --color: #123456; 
  --terra-button-active-background-color-default: rgb(var(--color), 0.2);
  --terra-button-active-border-color-default: rgb(var(--color), 0.5)
  --terra-button-active-color-default: var(--color);  
  ....(all 34 color variables)
}

.concept1 { --color: #qwe123; }
.concept2 { --color: #123qwe; }
...
alex-bezek commented 7 years ago

@zhongyn that css looks fine to me. Can you add a section on how this will be consumed. I have heard a few different idea's back and forth during the war room and chatting with you/mike.

The AC I can think of would be:

Thanks

mhemesath commented 7 years ago
.concept1 { --color: #qwe123; }
.concept2 { --color: #123qwe; }

I'm assuming we won't use --color but rather --terra-consumer-color?

alex-bezek commented 7 years ago

@zhongyn it looks like Mike is actually looking into how to consume these themes here https://github.com/cerner/terra-core/issues/723

zhongyn commented 7 years ago

@mhemesath we won't use --color, it's just pseduo code

zhongyn commented 7 years ago

@alex-bezek working on the button variables mapping now. For consuming these themes, originally i was thinking of adding terra-consumer button in terra-consumer project like:

terra-consumer-button/src/Button.jsx

import TerraButton from 'terra-button';
import 'terra-button/theme/consumer-button-themes.css';

propTypes = {
   theme: one of ['concept1', 'concept2',...]
}

defaultProps = {
  theme: ''
}

const Button = ({theme, ...otherProps}) => (
    <TerraButton className={`consumer-button-theme ${theme}`} ...otherProps}
)
mhemesath commented 7 years ago

Buttons don't need classes to theme... theming variables cascade. We need simply wrap the buttons with a div that activates the themign colors to that selector

zhongyn commented 7 years ago

is it based on the strategy of using theme provider component to swap class name at the run-time? if so, how would it work if we need two different concept buttons on the same page? Do we need to wrap each button with theme provider everywhere in the application? The end goal i think is for app developer to create a theme button easily.

alex-bezek commented 7 years ago

@zhongyn , depending on where we land with the Theming Implementation spike, it could end up something like 1) Default theme specified in webpack config, along with other bundled themes (default would be the terra-consumer theme for example) 2) Use a theme provider, or some wrapper div to set the whole current page to a particular theme/concept color. Can also be used to dynamically change themes of a page at run time (light/dark) 3) Individual components could be wrapped in their own provider/div that sets the variables to the scope under it.

You can have 1 wrapper component at the top level of the page, and then additional wrapper nested under it as the css variables should be scoped to that section I think.

zhongyn commented 7 years ago

3) Individual components could be wrapped in their own provider/div that sets the variables to the scope under it.

so it would still require wrapping individual components in their own provider. If wrapped in a div, how does it set the variables? another provider wrapping the div? it would end up adding classes at one of levels, no matter it's in button, div, or provider.

The above design actually allows variable cascade. The defaultProp theme is an empty string so it doesn't specify the --color. The value of --color will be whatever is set on the top level. With that in mind we will have a new design of theme class:

.consumer-button-theme {
  .concept1 { --terra-consumer-color: #qwe123; }
  ...
  --col: var(--terra-consumer-color, #123456);
  --terra-button-active-background-color-default: rgb(--col, 0.2);
  --terra-button-active-border-color-default: rgb(--col, 0.5)
  --terra-button-active-color-default: --col;  
  ....(all 34 color variables)
}
zhongyn commented 7 years ago

terra-consumer-button is compatible with theme provider at a top level, e.g:

<Provider theme="abs">
   ....
   <TerraConsumerButton />
</Provider>

the theme "abs" will cascade into TerraConsumerButton since it doesn't specify its own theme.

Consider TerraConsumerButton as a shortcut of:

<Provider theme="concept1">
   <TerraButton/>
</Provider>
alex-bezek commented 7 years ago

I'm not sure we want to release a whole other component/package just for this shortcut when wrapping a single button. If it's just applying a classname, can't we just do <TerraButton class='concept1' /> ?

mhemesath commented 7 years ago

I'm not sure we want to release a whole other component/package just for this shortcut when wrapping a single button. If it's just applying a classname, can't we just do ?

Class names should be treated as implementation details. In reality the class won't be concept but rather terra-Consumer-theme-concept1

alex-bezek commented 7 years ago

Yep, I didn't mean the className specifically, but rather on the off-chance that a single component needs to be different, the className (or whatever comes of this convo https://github.com/cerner/terra-core/pull/732#discussion_r133544472 ) can be passed to a specific component. We agree though, UX shouldn't give designs where this is a use case, but it can be a safety hatch for us.

zhongyn commented 7 years ago

Related PR: https://github.com/cerner/terra-core/pull/743

neilpfeiffer commented 7 years ago

Issue Update Changing labels from "Needs UX Review" to "Needs Design Input" and keeping the issue in the "Blocked/Needs Design Input".

This is from a meeting with the Anna, where she stated that after Human Factors testing, the design direction has changed where OCS:Consumer will no longer have concept-theme colored button variations, but will only a have single color scheme for the Consumer theme. New design callouts will be created with updated theme details and rework is blocked until we have details.

Affects issue #729 and PR #743

bjankord commented 6 years ago

This will be covered in https://github.com/cerner/terra-core/issues/1038