feat(menu): color prop and complex scheme #756

bmdalex · 2019-01-22T09:47:03Z

feat(menu): color scheme

Description

This PR:

continues the work started in feat(menu): color prop #712
adds color prop to Menu component
implements complex color scheme
improves color palette
creates Menu color examples
fixes menu bugs
- missing background color
- wrong color for different states

API

<Menu color={COLOR} .../>

where COLOR can be:

a string: 'primary' | 'secondary' | 'blue' | 'green' | 'grey' | 'orange' | 'pink' | 'purple' | 'teal' | 'red' | 'yellow' | string
an object with:
- key: one of 'foreground' | 'background' | 'border' to identify the target area of the component
- value: one of the strings above to describe the color of the area
  e.g.:

<Menu color={ foreground: 'pink', background: 'yellow', border: 'green' }} .../>

renders:

List of colored menu examples:

1. Regular theme

2. Dark theme

bmdalex · 2019-01-22T11:35:20Z

docs/src/examples/components/Menu/Variations/MenuExampleColor.shorthand.tsx

+  />
+)
+
+export default MenuExampleColor


for tracking @layershifter 's comment in: #712 (comment)

This example looks overcomplicated to me, may be we can:

introduce a new Select knob

use it to get select an active color

will address

Bare minimum examples

Reference SUIR for examples of clean Menu color examples. Let's not put so much information in a single example.

https://react.semantic-ui.com/collections/menu/#variations-colored

Definition only, avoid large permutations

The doc page should only be a "definition" of the component right now. That is a single example for each thing the menu can do, usually one example per prop.

At this point, let's not add examples for all possible permutations of what the menu can do.

@levithomason
in SUIR there are not that many versions of Menus; I just introduced a Select Knob at @layershifter 's proposal and now we have examples that look like this:

TODO - introduce screenshot

We can separate these into different example files where we reuse the knob or we can add a knob on every example for the color, but because of the way knobs are implemented we need to have a .knobs.tsx file for every example, which is a lot of duplication

what do you think?

codepretty · 2019-01-22T20:42:58Z

src/components/Menu/MenuItem.tsx

@@ -229,6 +233,7 @@ class MenuItem extends AutoControlledComponent<ReactProps<MenuItemProps>, MenuIt
          {Menu.create(menu, {
            defaultProps: {
              accessibility: submenuBehavior,
+              color,
              vertical: true,
              primary,


Should we remove primary and secondary props in MenuItem and Menu.tsx? And references to secondary in menuItemStyles as part of this change?

we can do that, but I wanted to have a separate PR for it; it also needs to be discussed again; I'll update the comment after we sync

codepretty · 2019-01-22T21:06:37Z

src/themes/teams/components/Menu/menuVariables.ts

    color: siteVars.gray02,
+    focusedColor: siteVars.black,
+    disabledColor: siteVars.gray06,
+    primaryActiveColor: siteVars.white,


Would it make more sense to get rid of specific variables like this primaryActiveColor and just set primary.foreground.active to siteVars.white directly?

Flat variables are preferred for now for consistency, ease of merging, and ease of discovery.

Once we formalize the structure of our design system, we might have nested values like this. For now, let's keep consistent and flat.

Okay, does that mean we should have the flat variable inherit from the nested color scheme values? Or get rid of the color scheme nested values altogether?

We're going to have a meeting on color in the morning. This needs more discussion that is suitable in this PR's comments. It will likely be early, would you like to join?

Add me to the invite. I would like to join if I can :)

to be updated after we discuss

codepretty · 2019-01-22T21:24:36Z

src/themes/teams/components/Menu/menuVariables.ts

-    primaryUnderlinedBorderColor: siteVars.gray08,
-
-    disabledColor: siteVars.gray06,
+    primaryActiveBorderColor: siteVars.brand,


You're using this for iconOnly colors instead of iconOnlyActiveColor, so this value should be what that was before (brand06) otherwise the style is incorrect in dark theme

will address after our sync

codepretty · 2019-01-22T21:29:00Z

src/themes/teams-high-contrast/components/Menu/menuVariables.ts

+
+    borderColor: siteVars.white,
+    primaryBorderColor: siteVars.white,
+    primaryActiveBorderColor: siteVars.white,


You're using this color for the icons color when being active, so it should match what activeColor is. Otherwise, the active state of the icon menu in high contrast is inaccessible

will address after our sync

codepretty · 2019-01-22T21:34:51Z

src/themes/teams/components/Menu/menuItemStyles.ts

  if (active && !underlined) return {}
+
+  if (underlined && !isFromKeyboard) {
+    return { color: v.focusedColor }


It's a little confusing because these variables are mixed with color scheme variables throughout the file. What is the right paradigm to follow? Do we use color scheme for everything, except when we need a specific exceptions or "override"? It looks like this might be the only time this variable is used. If so and if we're only using non-color scheme variables for specific instances, can we rename this variable to something specific like v.underlineInitialFocusedColor or something?

we need this to be a variable because this specific color should not change with the color prop (it's the same even if color is red or green)

regarding the name, it's debatable if we want to include something so specific... we need to come up with a naming convention for variables; if we take into account @codepretty 's proposal (underlineInitialFocusedColor) we can find the pattern to be:

${propNames}${state}${area}Color

where:

propNames: is a concatenation of props that are set to true for the branch the color is used;

state: is the state of the component (initial, focused, hovered, active, disabled, etc)

area: is the area of the component that gets the color (border, background, foreground, etc)

codepretty · 2019-01-22T21:35:53Z

src/themes/teams-dark/components/Menu/menuVariables.ts

+    primaryActiveColor: siteVars.black,
+    primaryFocusedColor: siteVars.black,
+
+    backgroundColor: siteVars.black,
  }
 }


Why do we not need color scheme in this file, but we do for the contrast variables?

Before adding any more variables, let's nail down exactly why we need certain variables and make them consistent across components. To do this, we need user stories and requirements to fulfill.

We should probably have a focused effort on reviewing components and the usefulness of certain variables across many components. Once we have requirements and have spent some time designing a variable scheme to fill the requirements, we can then start adding more variables.

levithomason

This complex color prop design is not inline with the current vision or principles. See #647 (comment).

Let's have a sync before merging any more of these changes.

bmdalex · 2019-02-22T10:23:44Z

Closing for now.

Reasons:

this work not yet aligned with current priorities
at the moment, the library is not ready to accept the complex changes this PR introduces

Findings and TODOs:

1. We need to drop complex color prop design and go back to the simpler API:

<Menu color='string' ... />

TODO:

remove complex color prop
stick to color prop with string values

2. Flat variable names vs nested color scheme (comment: #756 (comment)) :

the idea of the color scheme itself is good and should be used in our approach after we simplify it
not all components have the same color scheme so we should start from defining color schemes for individual components and then try find patterns and extract these into more complex logic
there's too much abstraction in renderComponent, see generateColorScheme

TODO:

for now we'll remove the color scheme from variables, because we're not ready for the concept of nested variables (needs more research and experimentation); we don't want to allow users to customize something like this yet
color scheme will be moved to the styles file of each component

3. How do we handle colors values that depend on prop values?

Right now, the color scheme structure doesn't support that; it identifies only colors, areas (background, border, etc) and states (initial, hovered, active, etc)

TODO:

rework the PR to make it the color scheme specific for each component
we will use a nested structure on the lines of: colors[colorName][area][state][prop], where:
- colors is the variable for colors;
  - colorName is one of the names of colors from palette ('primary' | 'secondary' | 'blue' | 'green' | 'grey' | 'orange' | 'pink' | 'purple' | 'teal' | 'red' | 'yellow' | string)
- area is the component area the color belongs to ('foreground' | 'background' | 'border', etc)
- state is the UI state of the component (initial, focused, hovered, active, disabled, etc)
- prop is an identifier for menu prop combinations that are true and need different variants (active background primary color is different than underlined

4. `primary` as name for prop and color is confusing

TODO:

rename primary color to brand

Color Scheme Proposal:

const colorScheme = {
  primary: {
    foreground: {
      initial: colorVariants[XXX],
      active: colorVariants[XXX],,
      disabled: colorVariants[XXX],,
      focused: colorVariants[XXX],,
      hovered: colorVariants[XXX],,
    },
    background: {
      // same props as above
    },
    border: {
      // same props as above
    },
    shadow: {
      // same props as above
    },
  },

  red: {
    // same props as above
  },

   // rest of colors
}

bmdalex requested review from dvdzkwsk, kuzhelov, layershifter, levithomason, miroslavstastny and mnajdova as code owners January 22, 2019 09:47

bmdalex mentioned this pull request Jan 22, 2019

feat(menu): color prop #712

Closed

bmdalex added the 🚀 ready for review label Jan 22, 2019

bmdalex force-pushed the feat/menu-color-scheme branch from ce57b6c to 111b00f Compare January 22, 2019 11:31

bmdalex commented Jan 22, 2019

View reviewed changes

codepretty reviewed Jan 22, 2019

View reviewed changes

levithomason suggested changes Jan 22, 2019

View reviewed changes

bmdalex force-pushed the feat/menu-color-scheme branch 4 times, most recently from 10f9d42 to f0e52fb Compare January 29, 2019 14:33

bmdalex added 4 commits January 30, 2019 16:29

feat(menu): color prop and complex scheme

995bcbe

changelog

7d08a74

implemented knobs for the examples

1146515

improved examples and select knob

c449916

bmdalex force-pushed the feat/menu-color-scheme branch from f0e52fb to c449916 Compare January 30, 2019 15:30

bmdalex added postponed Item is postponed and will be reconsidered in future and removed 🚀 ready for review labels Feb 5, 2019

bmdalex closed this Feb 22, 2019

bmdalex mentioned this pull request Mar 18, 2019

feat(alert): v1 #1063

Merged

bmdalex deleted the feat/menu-color-scheme branch April 9, 2019 14:22

feat(menu): color prop and complex scheme #756

feat(menu): color prop and complex scheme #756

Uh oh!

Conversation

bmdalex commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

feat(menu): color scheme

Description

API

List of colored menu examples:

1. Regular theme

2. Dark theme

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Bare minimum examples

Definition only, avoid large permutations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codepretty Jan 22, 2019 • 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!

codepretty Jan 22, 2019 • 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!

codepretty Jan 22, 2019 • 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!

levithomason left a comment

Choose a reason for hiding this comment

Uh oh!

bmdalex commented Feb 22, 2019

Closing for now.

Reasons:

Findings and TODOs:

1. We need to drop complex color prop design and go back to the simpler API:

TODO:

2. Flat variable names vs nested color scheme (comment: #756 (comment)) :

TODO:

3. How do we handle colors values that depend on prop values?

TODO:

4. primary as name for prop and color is confusing

TODO:

Color Scheme Proposal:

Uh oh!

Uh oh!

bmdalex commented Jan 22, 2019 •

edited

Loading

levithomason Jan 22, 2019 •

edited

Loading

codepretty Jan 22, 2019 •

edited

Loading

codepretty Jan 22, 2019 •

edited

Loading

codepretty Jan 22, 2019 •

edited

Loading

4. `primary` as name for prop and color is confusing