feat(variants): Add variants to compose. #2281
Conversation
| }, | ||
| variants: { | ||
| strong: Variant.boolean({ fontWeight: 700 }), | ||
| rounded: Variant.boolean({ borderRadius: 30 }), |
There was a problem hiding this comment.
Just to reiterate some verbal discussion we had, I think it'd be neat if variants could also support slot implementations at component creation time rather than have to be passed in at render time.
There was a problem hiding this comment.
This looks pretty good. One thought I have - do we have a perf test validating the compose examples? Could we maybe hook one up to Checkbox proto or Slider or something?
| @@ -0,0 +1,103 @@ | |||
| import React from 'react'; | |||
|
|
|||
| import { initalize } from './../..'; | |||
There was a problem hiding this comment.
make sure that file paths are explicit. This import would break an AMD build. (In fabric we have a build step that validates file path imports go to explicit files because of this nuance.)
| ) => { | ||
| let classes = classNamesCache.get(theme); | ||
| let classes = null; // classNamesCache.get(theme); |
There was a problem hiding this comment.
I don't think this was intentional.
There was a problem hiding this comment.
For now it is, as this PR doesn't yet implement correct caching based on props. Will be in a following PR once we ratify the design of variants. Good callout though!
| }; | ||
| }, | ||
| variants: { | ||
| strong: Variant.boolean({ fontWeight: 700 }), |
There was a problem hiding this comment.
This looks interesting, however sometimes in the definition of the styles we will want to specify something if some variant is not defined, how would that work?
There was a problem hiding this comment.
For lack of a variant, the styles will default to whatever was defined in the base set of tokens. It would be easy to define another style of variant (eg booleanAlwaysEvaluated) that gets a chance to affect the tokens even when not called.
| }, | ||
| }; | ||
| }, | ||
| variants: { |
There was a problem hiding this comment.
I can see that this is inspired from the work we have done on #2200 however after playing a bit with it, I have some doubts about how it would work when you will need to combine variants together. More over, usually for some variants like disable, you want to override everything that was previously defined (like no hover styles for example), would lieke to hear from you how you envision these to work. Generally, I think it would be useful to see more complex styles examples that will require the above mention scenarios.
There was a problem hiding this comment.
Added another scenario with a disabled prop. The styles implementation isn't ideal (yet), but shows what is possible.
composeimplementationreact-themingpackage for inner loopstill not done: