Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(variants): Add variants to compose. #2281

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/react-theming/.storybook/config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { configure } from '@storybook/react';
import { withInfo } from '@storybook/addon-info';
import { addDecorator } from '@storybook/react';

addDecorator(withInfo());

const req = require.context('../src', true, /\.stories\.tsx$/);

function loadStories() {
return req.keys().map(req);
}

configure(loadStories, module);
2 changes: 2 additions & 0 deletions packages/react-theming/.storybook/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
const webpackConfig = require('@fluentui/scripts/config/storybook/webpack.config');
module.exports = webpackConfig;
1 change: 1 addition & 0 deletions packages/react-theming/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"build": "just-scripts build",
"clean": "just-scripts clean",
"just": "just-scripts",
"start": "just-scripts start",
"start-test": "just-scripts test:watch",
"test": "just-scripts test",
"update-snapshots": "just-scripts jest:snapshots"
Expand Down
43 changes: 34 additions & 9 deletions packages/react-theming/src/compose.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
import { useTheme } from './themeContext';
import { resolveTokens } from './resolveTokens';
import jss from 'jss';

import { resolveTokens } from './resolveTokens';
import { ITheme } from './theme.types';
import { useTheme } from './themeContext';
import { Variant } from './variant';

type Options = ComposeOptions[];
type SlotsAssignment = any;
type Variants = { [variantName: string]: Variant };

interface ComposeOptions {
name?: string;
slots?: any;
tokens?: any;
styles?: any;
variants?: Variants;
}

export interface Composeable {
Expand All @@ -26,6 +30,7 @@ export type ForwardRefComponent<TProps, TElement> = React.FunctionComponent<
interface ComposedFunctionComponent<TProps> extends React.FunctionComponent<TProps> {
__optionsSet?: ComposeOptions[];
__directRender?: React.FunctionComponent<TProps>;
variants?: Variants;

// Needed for components using forwardRef (See https://github.com/facebook/react/issues/12453).
render?: React.FunctionComponent<TProps>;
Expand Down Expand Up @@ -60,7 +65,7 @@ export const _composeFactory = (useThemeHook: any = useTheme) => {

return renderFn({
...props,
classes: _getClasses(componentName, theme, classNamesCache, optionsSet),
classes: _getClasses(componentName, theme, classNamesCache, optionsSet, props),
slots,
});
};
Expand All @@ -70,6 +75,7 @@ export const _composeFactory = (useThemeHook: any = useTheme) => {
}

Component.propTypes = baseComponent.propTypes;
Component.variants = options.variants;

Component.__optionsSet = optionsSet;
Component.__directRender = renderFn;
Expand Down Expand Up @@ -130,20 +136,39 @@ const _mergeOptions = (options: ComposeOptions, baseOptions?: Options): Options
return optionsSet;
};

/**
* _tokensFromOptions returns the accurate set of tokens
* based on the currently rendered props.
*
* @internal
*
* @param options A set of options
* @param props Props for this render
*/
export const _tokensFromOptions = (options: any[], props: any) => {
let result = options.reduce((acc, option) => {
return { ...acc, ...(option.tokens || {}) };
}, {});
options.forEach(option => {
Object.keys(option.variants || {}).forEach(variantName => {
const v: Variant = option.variants[variantName];
result = { ...result, ...v.tokens(props[variantName]) };
});
});
return result;
};

const _getClasses = (
name: string | undefined,
theme: ITheme,
classNamesCache: WeakMap<any, any>,
optionsSet: any[],
props: any,
) => {
let classes = classNamesCache.get(theme);
let classes = null; // classNamesCache.get(theme);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this was intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


if (!classes) {
const tokens = resolveTokens(
name,
theme,
optionsSet.map(o => o.tokens || {}),
);
const tokens = resolveTokens(name, theme, _tokensFromOptions(optionsSet, props));
let styles: any = {};

optionsSet.forEach((options: any) => {
Expand Down
103 changes: 103 additions & 0 deletions packages/react-theming/src/examples/compose/index.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import React from 'react';

import { initalize } from './../..';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

import { ThemeProvider } from './../../components/ThemeProvider/ThemeProvider';
import { compose } from './../../compose';
import { Variant } from './../../variant';
import { ITheme, IColorRamp } from '../../theme.types';

initalize();

export default {
component: 'compose',
title: 'Compose Demos',
};

const emptyRamp: IColorRamp = { values: [], index: -1 };
const theme: ITheme = {
components: {},
colors: {
background: 'white',
bodyText: 'black',
subText: 'black',
disabledText: 'green',
brand: emptyRamp,
accent: emptyRamp,
neutral: emptyRamp,
success: emptyRamp,
warning: emptyRamp,
danger: emptyRamp,
},
fonts: {
default: '',
userContent: '',
mono: '',
},
fontSizes: {
base: 1,
scale: 1,
unit: 'rem',
},
animations: {
fadeIn: {},
fadeOut: {},
},
direction: 'ltr',
spacing: {
base: 0,
scale: 0,
unit: 'rem',
},
radius: {
base: 0,
scale: 0,
unit: 'rem',
},
icons: {},
schemes: {},
};

const BaseDiv: React.FunctionComponent<{ classes: any }> = props => {
return <div className={props.classes.root}>{props.children}</div>;
};

const ComposedDiv = compose(BaseDiv as any, {
tokens: {
fontWeight: 300,
borderRadius: 0,
},
styles: (tokens: any) => {
return {
root: {
background: 'red',
fontWeight: tokens.fontWeight,
borderRadius: tokens.borderRadius,
margin: 10,
padding: 10,
},
};
},
variants: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another scenario with a disabled prop. The styles implementation isn't ideal (yet), but shows what is possible.

strong: Variant.boolean({ fontWeight: 700 }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rounded: Variant.boolean({ borderRadius: 30 }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

},
});

export const composedDemo = () => (
<ThemeProvider theme={theme}>
<ComposedDiv>I am children</ComposedDiv>
</ThemeProvider>
);

export const variantDemo = () => {
return (
<ThemeProvider theme={theme}>
<ComposedDiv>Default control</ComposedDiv>
<ComposedDiv strong>Strong variant</ComposedDiv>
<ComposedDiv rounded>Rounded variant</ComposedDiv>
<ComposedDiv strong rounded>
Strong &amp; rounded variants
</ComposedDiv>
</ThemeProvider>
);
};
2 changes: 1 addition & 1 deletion packages/react-theming/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ export { ThemeProvider } from './components/ThemeProvider/ThemeProvider';
export { Box } from './components/Box/Box';
export { createTheme } from './utilities/createTheme';

jss.setup(preset());
export const initalize = () => jss.setup(preset());
42 changes: 18 additions & 24 deletions packages/react-theming/src/resolveTokens.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const reifyColors = (partial: Partial<IThemeColorDefinition>): IThemeColorDefini

describe('resolveTokens', () => {
it('can resolve a literal', () => {
expect(resolveTokens('', reifyTheme({}), [{ value: 'abc' }])).toEqual({
expect(resolveTokens('', reifyTheme({}), { value: 'abc' })).toEqual({
value: 'abc',
});
});
Expand All @@ -41,26 +41,22 @@ describe('resolveTokens', () => {
},
}),
}),
[
{
value: (_: any, t: ITheme) => t.colors.brand.values[t.colors.brand.index],
},
],
{
value: (_: any, t: ITheme) => t.colors.brand.values[t.colors.brand.index],
},
),
).toEqual({ value: '#bbb' });
});

it('can resolve a token related to another', () => {
expect(
resolveTokens('', reifyTheme({}), [
{
value: 'abc',
value2: {
dependsOn: ['value'],
resolve: ([value]: any, theme: any) => `${value}def`,
},
resolveTokens('', reifyTheme({}), {
value: 'abc',
value2: {
dependsOn: ['value'],
resolve: ([value]: any, theme: any) => `${value}def`,
},
]),
}),
).toEqual({ value: 'abc', value2: 'abcdef' });
});

Expand All @@ -76,15 +72,13 @@ describe('resolveTokens', () => {
},
}),
}),
[
{
value2: {
dependsOn: ['value'],
resolve: ([value]: any, theme: ITheme) => `${value}def`,
},
value: (_: any, t: ITheme) => t.colors.brand.values[0],
{
value2: {
dependsOn: ['value'],
resolve: ([value]: any, theme: ITheme) => `${value}def`,
},
],
value: (_: any, t: ITheme) => t.colors.brand.values[0],
},
),
).toEqual({ value: '#aaa', value2: '#aaadef' });
});
Expand All @@ -100,7 +94,7 @@ describe('resolveTokens', () => {
},
},
});
expect(resolveTokens('MyComponent', theme, [{ value: 'foo' }])).toEqual({
expect(resolveTokens('MyComponent', theme, { value: 'foo' })).toEqual({
value: 'bar',
});
});
Expand All @@ -125,7 +119,7 @@ describe('resolveTokens', () => {
resolve: ([value]: any, theme: ITheme) => `${value}foo`,
},
};
const result = resolveTokens('MyComponent', theme, [baseTokens]);
const result = resolveTokens('MyComponent', theme, baseTokens);
expect(result).toEqual({ value: 'foo', value2: 'foobar' });
});
});
Expand Down
11 changes: 5 additions & 6 deletions packages/react-theming/src/resolveTokens.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ITheme } from './theme.types';

export type TokenDictShorthand = { [name: string]: any };
type TokenDict = { [name: string]: IToken };

interface IToken {
Expand Down Expand Up @@ -79,14 +80,12 @@ class TokenFactory {
* @param sourceTokensSet
* @internal
*/
export const resolveTokens = (name: string | undefined, theme: ITheme, sourceTokensSet: any[]) => {
export const resolveTokens = (name: string | undefined, theme: ITheme, sourceTokens: any) => {
const tokens: TokenDict = {};

sourceTokensSet.forEach(sourceTokens => {
for (const tokenName in sourceTokens) {
tokens[tokenName] = TokenFactory.from(tokens, sourceTokens[tokenName], tokenName);
}
});
for (const tokenName in sourceTokens) {
tokens[tokenName] = TokenFactory.from(tokens, sourceTokens[tokenName], tokenName);
}

if (name && theme.components[name] && theme.components[name].tokens) {
const sourceTokens = theme.components[name].tokens;
Expand Down
Loading