feat: support theming and styling by levithomason · Pull Request #16 · microsoft/fluent-ui-react

levithomason · 2018-07-27T01:14:51Z

Apologies, this PR is necessarily large:

Move rules and variables to themes/teams
Rename "rules" to "styles"
Add support for nested theming
Add support for theme switching
Add typings for all theming

Relevant features...

Styles prop

The styles prop is now supported:

<Button 
  content="I am gold"
  styles={{ root: { color: 'goldenrod' } }}
/>

Nested Providers

You can now nest Providers and themes will be merged:

<Provider theme={teamsTheme}>
  <Button content="Default Button" />

  <Provider
    theme={{
      componentVariables: {
        Button: { backgroundColor: 'goldenrod' }
      }
    }}
  >
    <Button content="Golden Button" />
  />
</Provider>

mnajdova · 2018-07-30T10:11:45Z

I fixed some bugs regarding merging the component's variables and styles. The values that we receive from the theme are in the following format:

componentVariables = [
  [
    Avatar: (...)
  ],
  [
    Avatar: (...)
  ]
]

so I introduced some helpers methods for converting these lists to one array of callables and another one for merging the callables. The variables and the styles can now be overridden when using the components by providing the variables and styles props.

Let me know if you have any feedback or need some more help with this.

levithomason · 2018-08-02T18:26:21Z

Thanks for wanting to help. Note, I had a lot of unpushed changes that included dealing with merging themes downstream on context. I've forced pushed those to save the work and resume progress here.

I will fix conflicts and continue.

kuzhelov · 2018-08-06T15:48:42Z

docs/src/index.tsx

-    style: { fontWeight: 700 },
-  },
-]
+console.log('THEME', theme)


seems to be a debug leftover

Thanks, branch was still in progress until right now 👍

codecov · 2018-08-06T22:22:54Z

Codecov Report

Merging #16 into master will increase coverage by 0.32%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   85.97%   86.29%   +0.32%     
==========================================
  Files          76       39      -37     
  Lines        1112      664     -448     
  Branches      228       90     -138     
==========================================
- Hits          956      573     -383     
+ Misses        149       88      -61     
+ Partials        7        3       -4

Impacted Files	Coverage Δ
src/components/Header/Header.tsx	`94.73% <ø> (-0.92%)`	⬇️
src/components/Button/Button.tsx	`81.08% <ø> (-1.85%)`	⬇️
src/components/Chat/Chat.tsx	`94.73% <ø> (-0.51%)`	⬇️
src/components/Accordion/Accordion.tsx	`66% <ø> (-1.31%)`	⬇️
src/components/List/ListItem.tsx	`82.35% <ø> (-1.29%)`	⬇️
src/components/Avatar/Avatar.tsx	`91.17% <ø> (-0.93%)`	⬇️
src/components/Layout/Layout.tsx	`88% <ø> (-0.47%)`	⬇️
src/components/Icon/Icon.tsx	`79.16% <ø> (-2.98%)`	⬇️
src/components/Accordion/AccordionContent.tsx	`80% <ø> (-4.22%)`	⬇️
src/components/Menu/Menu.tsx	`100% <ø> (ø)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f14208b...f12091f. Read the comment docs.

levithomason · 2018-08-06T22:43:04Z

src/components/Avatar/Avatar.tsx

                name={icon}
                color="white"
-                style={avatarRules.presenceIcon()}
+                className={classes.presenceIcon}


This looks like a bug, will update.

levithomason · 2018-08-06T22:47:06Z

src/components/Label/Label.tsx

-      ? callable(labelPropVariables)()
-      : callable(Label.variables)()
+    const iconVariables = callable(iconProps.variables)() || {}
+    const labelVariables = callable(variables)() || {}


This may also need careful review.

The iconProps available at this point are produced by the shorthand factory. Any variables provided on iconProps at this point will not have access to the theme values on context so they cannot be resolved before being handed to the overrides function.

This means we will not be able to hand resolved variables to components reliably, unless we also make factories aware of how to resolve variables (which I think is a very bad idea). That would require making factories access context.

/cc @kuzhelov @mnajdova

To be clear, I don't have a solution or proposal for this right now but I think this PR needs to get merged. It is too difficult to maintain merge conflict fixes if we push it out.

yes, agree with increasing complexity to address merge conflicts - will help you with merging this PR today.

levithomason · 2018-08-06T22:48:58Z

src/components/List/listVariables.ts

-  vars.contentLineHeight = siteVars.lineHeightSmall
-
-  return vars
-}


Moving this particular file represents a significant change. The ListItem variables were defined in the listVariables file, making them accessible on context under List instead of ListItem. I had to rename this file to listItemVariables and expose it on context as ListItem in order for our theming contract to work (components get variables and styles by their display name).

levithomason · 2018-08-06T22:57:30Z

src/components/Provider/Provider.tsx

+            <RendererProvider renderer={outgoingTheme.renderer}>
+              <ThemeProvider theme={outgoingTheme}>{children}</ThemeProvider>
+            </RendererProvider>
+          )


This is important. Each Provider now:

consumes the theme from context

merges it with its theme prop

passes the merged theme downstream

This enables nested cascading themes.

levithomason · 2018-08-06T23:03:05Z

src/lib/mergeThemes.ts

+  }, target)
+}
+
+const mergeThemes = (...themes: IThemeInput[]): IThemePrepared => {


This function is best reviewed by looking at the tests, mergeThemes-test.ts.

levithomason · 2018-08-06T23:03:14Z

src/lib/rem.tsx

-  _htmlFontSize = fontSize || getComputedFontSize()
-}
-
-export default rem


dvdzkwsk · 2018-08-07T00:19:42Z

src/components/Provider/Provider.tsx

-      PropTypes.oneOfType([PropTypes.string, PropTypes.object, PropTypes.func]),
-    ),
-    rtl: PropTypes.bool,
+    theme: PropTypes.shape({


Is this required? It looks to be based on the interface.

Currently, docs are still generated from propTypes. That needs to be moved to being generated from typings. Then we can consider removing propTypes.

The other consideration is that most consumers are not running typescript. So, I think we may end up keeping both interfaces and propTypes around. Also, some runtime checks are really handy since we can do things with values (suggest icon names, etc).

Sorry, I meant to say "should the theme propType be .isRequired?". I wasn't talking about the propTypes themselves.

Got it, thanks. We have a scheduled task to make a default theme. For now, it is required.

dvdzkwsk · 2018-08-07T00:23:03Z

src/lib/getClasses.ts

+
+  // root, icon, etc.
+  const componentParts: string[] = stylesArr.reduce((acc, next) => {
+    return next ? _.union(acc, _.keys(next)) : acc


next should always be truthy because of the above call to toCompactArray, if I'm understanding this correctly.

dvdzkwsk · 2018-08-07T00:24:05Z

src/lib/getClasses.ts

+
+/**
+ * Returns a string of HTML classes.
+ * Renders one or many component styles (objects of component parts) to the DOM.


What is this line about rendering to the DOM?

dvdzkwsk · 2018-08-07T00:33:20Z

src/lib/mergeThemes.ts

+  }, target)
+}
+
+const mergeThemes = (...themes: IThemeInput[]): IThemePrepared => {


Nit: I think this would be more simple if it took an array of themes.

It doesn't really take multiple arguments. It takes a single collection of themes. Representing this with an appropriate data type makes this obvious.

It becomes possible to add additional arguments if needed.

It makes it easier to provide a list of themes to this function. You can do mergeThemes(...myListOfThemes), but I would ask "why?" when you can just do mergeThemes(myListOfThemes).

Least importantly, it saves you casting arguments to an array.

Justified myself because you asked :).

dvdzkwsk · 2018-08-07T00:40:13Z

src/lib/mergeThemes.ts

+    acc.rtl = mergeRTL(acc.rtl, next.rtl)
+
+    // Use the correct renderer for RTL
+    acc.renderer = acc.rtl ? felaRtlRenderer : felaRenderer


Should emptyTheme have a default renderer? It looks to be possible to never set renderer if next is never truthy. Same goes for rtl.

dvdzkwsk · 2018-08-07T00:45:51Z

src/lib/toCompactArray.ts

@@ -0,0 +1,5 @@
+const toCompactArray = <T = any>(...values: T[]): T[] => {
+  return [].concat(...values).filter(Boolean)


You shouldn't have to spread values here. Also, why not _.compact?

Spread is required as each value may be a single value or an array. we need to concat each argument.

_.compact only compacts a single array, it is essentially equivalent to [].filter(Boolean).

The feature I need is:

Cast to array

Allow items or arrays of items

Compact the resulting array

So:

toCompactArray(1) //=> [1] toCompactArray(1, null, 2) //=> [1, 2] toCompactArray(1, 'two', [3, 4], null, ['more', 'stuff', undefined]) // => [1, 'two', 3, 4, 'more', 'stuff']

A more literal name would be something quite verbose like castToFlattenedCompactArray() since it does all of these things.

You could also create it from _.compact(_.flatten(...values)), however, given how often it is used and on hot paths I went with a minimal custom function that we can more easily optimize. Lodash does a lot of fancy checks and handling that we don't need to waste more cycles on.

Example, we can already optimize this by using reduce and skipping the second loop via filter. We can just not concat empty values into the accumulator. You could also put an early return check for values.length === 1 scenarios, etc...

Sure, but why can't you just pass an array when you call toCompactArray? 🤔

You'd still need to handle the flattening, of course.

dvdzkwsk · 2018-08-07T00:46:57Z

src/lib/renderComponent.tsx

+        componentVariables = {},
+        componentStyles = {},
+        rtl = false,
+        renderer = felaRenderer,


See my previous comment about missing values for rtl and renderer. It would be a benefit to overall maintainability if the provider did all of the heavy lifting, and these consumers could be more confident about the interface they receive, since they don't have to work with all the same uncertainty.

dvdzkwsk · 2018-08-07T00:48:56Z

src/themes/teams/components/Accordion/accordionStyles.ts

+import { IComponentPartStylesInput, ICSSInJSStyle } from '../../../../../types/theme'
+
+const accordionStyles: IComponentPartStylesInput = {
+  root: (): ICSSInJSStyle => ({


Would it perhaps be easier to type the overall styles object as [key: string]: (): ICSSInJSStyle (or whatever the dammed syntax is) so that every individual style function doesn't have to declare its type?

Yes, did this however it doesn't provide completions inside the style object for index keys. It will only do so if you type the return value. Even that has many shortcomings. Note the const accordionStyles: IComponentPartStylesInput, which is doing exactly what you are referring to.

Since this PR is being broken down into smaller bits, we'll save the typings for later.

dvdzkwsk · 2018-08-07T00:50:13Z

src/themes/teams/components/Button/buttonStyles.ts

-
-    return {
-      ...rules,
+      ...(disabled


Nit: may be more maintainable to break this out into another function rather than inlining it.

dvdzkwsk · 2018-08-07T00:51:31Z

src/themes/teams/components/Divider/dividerVariables.ts

@@ -0,0 +1,21 @@
+export interface IDividerVariables {
+  [key: string]: string | number


Why pin an unknown key to a specific type? Mostly just curious.

Just had to pick something and this seemed reasonable. This is essentially a "first guess" at what acceptable style variable values would be. Very much open to change and iteration.

We're targeting simple and flat variable objects. I can say I don't think we should allow arrays and objects and such. The theming system is pretty robust as it is, keeping variables simple will help make themes more usable.

kuzhelov · 2018-08-07T15:15:55Z

src/lib/callable.ts

@@ -1,3 +1,5 @@
-const callable = val => (typeof val !== 'function' ? () => val : val)
+const callable = (possibleFunction: any) => (...args: any[]) => {
+  return typeof possibleFunction === 'function' ? possibleFunction(...args) : possibleFunction


Can we attach a benchmark into JSDoc?
https://jsperf.com/startdust-callable

I thought that callableRest() would be faster 😼 But, not

levithomason · 2018-08-08T01:55:14Z

src/lib/renderComponent.tsx

+        const styleParam: ComponentStyleFunctionParam = {
+          props,
+          variables,
+          siteVariables,


⚠️ NOTE TO SELF

siteVariables should not be passed to style functions. This was found in the breakout work for moving rules and variables to themes/teams/components. The Text component was directly accessing site variables via an import. Instead, it needs to get these values from the textVariables.ts.

This is the only case of siteVariables being accessed in a component's style function. It should also be removed.

levithomason requested review from dvdzkwsk and kuzhelov as code owners July 27, 2018 01:14

levithomason force-pushed the feat/organize-theme branch 2 times, most recently from 0eb4db4 to 327b86c Compare July 27, 2018 15:40

levithomason requested a review from mnajdova as a code owner July 27, 2018 15:40

levithomason mentioned this pull request Aug 2, 2018

feat(Accordion): add props interface #33

Merged

levithomason force-pushed the feat/organize-theme branch from 418e947 to c6298fe Compare August 2, 2018 18:24

levithomason force-pushed the feat/organize-theme branch from c6298fe to 4464caf Compare August 2, 2018 23:33

levithomason added 2 commits August 2, 2018 17:08

move rules and vars to themes, add typings

06ae6d9

wip

a9bbbac

levithomason force-pushed the feat/organize-theme branch from 4464caf to a7e0599 Compare August 3, 2018 16:01

test: add mergeTheme tests

9328b1b

levithomason force-pushed the feat/organize-theme branch from a7e0599 to 9328b1b Compare August 4, 2018 03:25

kuzhelov reviewed Aug 6, 2018

View reviewed changes

merge master, resolve conflicts, fix tests

af4ed15

levithomason force-pushed the feat/organize-theme branch from 6e27732 to af4ed15 Compare August 6, 2018 19:13

levithomason added 2 commits August 6, 2018 15:14

fix: mv listVariables to listItemVariables

28a0270

fix lint error, add changelog entry

4b299a2

levithomason changed the title ~~[WIP] feat: support theming~~ feat: support theming Aug 6, 2018

levithomason changed the title ~~feat: support theming~~ feat: support theming and styling Aug 6, 2018

levithomason added 2 commits August 6, 2018 15:37

docs: restore default imports in doc examples

e136132

docs: cleanup cruft component-anatomy page

37d1d5a

levithomason commented Aug 6, 2018

View reviewed changes

levithomason added 2 commits August 6, 2018 15:50

fix: Provier theme propTypes

e837319

refactor: use toCompactArray where possible

bd201af

levithomason commented Aug 6, 2018

View reviewed changes

refactor: renderComponentStyles to getClasses

6b788f0

levithomason commented Aug 6, 2018

View reviewed changes

src/lib/rem.tsx

_htmlFontSize = fontSize || getComputedFontSize()

}

export default rem

Copy link

Member Author

levithomason Aug 6, 2018

Choose a reason for hiding this comment

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

Cruft

add todo for avatar icon work

b408da7

dvdzkwsk approved these changes Aug 7, 2018

View reviewed changes

kuzhelov reviewed Aug 7, 2018

View reviewed changes

levithomason mentioned this pull request Aug 7, 2018

Fixed import statement, for d.ts files #56

Closed

levithomason commented Aug 8, 2018

View reviewed changes

This was referenced Aug 8, 2018

refactor: move rules and variables to themes/teams #57

Merged

chore: introduce mergeThemes function #64

Merged

levithomason added 2 commits August 8, 2018 19:32

merge master, fix conflicts

5bd4b47

merge master

49f2bf6

levithomason mentioned this pull request Aug 9, 2018

refactor: simplify ContributionPrompt and remove customCSS #68

Merged

levithomason added 4 commits August 8, 2018 22:55

merge master

9c008a0

revert minor changes

2820a5f

rename getClasses

d3bfe32

rename getClasses with git mv

d24d07d

levithomason mentioned this pull request Aug 9, 2018

docs: cleanup examples #69

Merged

levithomason added 7 commits August 9, 2018 00:06

merge master

0fdb27e

uncomment icon styles

6143126

refactor: avatar styles, fix export name

812392f

refactor: add perf link to callable

48c5b96

feat: expose resolved variables/styles in renderComponent

c325f95

merge master

7218855

feat: pass resolved variables and styles to components

f12091f

levithomason merged commit e345edf into master Aug 9, 2018

levithomason deleted the feat/organize-theme branch August 9, 2018 23:05

		@@ -0,0 +1,5 @@
		const toCompactArray = <T = any>(...values: T[]): T[] => {
		return [].concat(...values).filter(Boolean)

		@@ -0,0 +1,21 @@
		export interface IDividerVariables {
		[key: string]: string \| number

Conversation

levithomason commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Styles prop

Nested Providers

Uh oh!

mnajdova commented Jul 30, 2018

Uh oh!

levithomason commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdzkwsk Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

⚠️ NOTE TO SELF

levithomason commented Jul 27, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading

levithomason Aug 6, 2018 •

edited

Loading

levithomason Aug 8, 2018 •

edited

Loading

dvdzkwsk Aug 7, 2018 •

edited

Loading