Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CSS-in-JS] Global theme #4643

Merged
merged 108 commits into from
Aug 10, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 15, 2021

This is a starting PR to work out the basic global theme object

Help

See outdated commentary _I'll also comment about/point to these in the code._
  1. I'm having some issues about where I'm using computed() and getting an error around 'getOwnPropertyDescriptor' like with the text variants if I try to pull them out of the theme.js file.
  2. I can't seem to declare themes with buildTheme() outside of theme.js
  3. color keys are getting removed/not outputting?
  4. Can't seem to pass any values that aren't a number or string, like if we want an array
  5. Trying to be efficient by mapping through arrays to create theme keys comes with some TS challenges
  6. Can we use useEuiTheme in helper functions to get something like the current base value when computing other theme values?
  7. Trying to just re-use a full object is not working. Like if the key is:
font.xs: {
  fontSize: computed();
  lineHeight: computed();
}

And I try to use it like this to spread those keys onto a new key, it doesn't work:

title.xs: {
  ...computed(
      [`${COLOR_MODE_KEY}.font.xs`],
      ([font]) => font
    ),
}

Discuss

COLOR_MODE_KEY

tl;dr: Using LIGHT and DARK keys at any level of nesting (#4643 (comment))

See outdated commentary

Can we get rid of having to specify [COLOR_MODE_KEY] which evaluates to colors in order to distinguish between light and dark theme values? Instead can the actual light and dark keys be those?

Before

const euiTheme = {
  [COLOR_MODE_KEY] = {
    light: {
      colors: {
        primary: '#hash'
      },
    },
    dark: {
      colors: {
        primary: '#hash'
      },
    }
  },
  size: {
    // This is nice because most likely isn't affected by light/dark mode, so you only have to specify once
  },
  focus: {
    // This is less so because instead of the consumer being able to just say `focus.background` they have to say `focus.colors.background`
    [COLOR_MODE_KEY] = {
      light: {
        background: computed('for light theme'),
      },
      dark: {
        background: computed('for dark theme'),
      },
    },
    size: {
      m: computed(),
    }
  }
};

Instead, it could be a top level key that either exists or doesn't (for single color mode)

const euiTheme = {
  light: {
    colors: {
      primary: '#hash'
    },
    size: {
      // This may not actually be affected by light/dark color mode, but it makes most sense as a creator of the theme and as a consumer to not have to split/create a `colors` key every time you want to adjust for light or dark mode
    },
    focus: {
      background: computed('for light theme'),
      size: {
        m: computed(),
      }
    }
  },
  dark: {
    colors: {
      primary: '#hash'
    },
    size: {
      // This may not actually be affected by light/dark color mode, but it makes most sense as a creator of the theme and as a consumer to not have to split/create a `colors` key every time you want to adjust for light or dark mode
    },
    focus: {
      background: computed('for dark theme'),
      size: {
        m: computed(),
      }
    }
  }
};

Or even better, just remove always check for light or dark at any nested level

const euiTheme = {
  colors: {
    light: {
      primary: '#hash',
    },
    dark: {
      primary: '#hash',
    },
  },
  size: {
    // This is nice because most likely isn't affected by light/dark mode, so you only have to specify once
  },
  focus: {
    // Internal components can specify light/dark even inside the key?
    background: {
      light: computed('for light theme'),
      dark: computed('for dark theme'),
    },
    size: {
      m: computed(),
    },
  },
};

Chakra uses hooks internally when creating the theme that would look something like this, though I'm not convinced this is the best approach or is able to work with computed values, just an example:

const euiTheme = {
  colors: {
    primary: useColorMode('#hashForLight', '#hashForDark'),
  },
  focus: {
    // This is certainly the most condensed/efficient way and most similar to our current SASS `lightOrDarkTheme()` function
    background: useColorMode('#hashForLight', '#hashForDark'),
    size: {
      m: computed(),
    },
  },
};

Global vs Component theme

We should limit the top level euiTheme to truly just the global tokens like color, size, type sizes, etc. Even things like euiFontWeightCode should be scoped to it's own theme.

Before:

const euiTheme = {
  colors: {
    primary: '#hash'
  },
  ...moreGlobals,
  button: {
    primary: {
      fill: computed(),
      color: computed(),
    },
    size: {
      m: computed(),
    }
  }
};

After:

const euiTheme = {
  colors: {
    primary: '#hash'
  },
  ...onlyGlobals,
};

// Only imported if consumers want to alter this specific "theme"
const euiButton = {
  primary: {
    fill: useEuiTheme(),
    color: useEuiTheme(),
  },
  size: {
    m: useEuiTheme(),
  }
}

Checklist Ignoring for now, will go through it all on the feature branch

cchaos added 14 commits February 24, 2021 16:15
commit 9f4bcba
Merge: e7c9c43 88709ac
Author: Greg Thompson <[email protected]>
Date:   Thu Mar 11 10:41:10 2021 -0600

    Merge branch css-in-js/sizes of https://github.com/thompsongl/eui into css-in-js/sizes

commit e7c9c43
Merge: 60ef501 1a35553
Author: Greg Thompson <[email protected]>
Date:   Thu Mar 11 10:38:44 2021 -0600

    Merge branch feature/css-in-js into css-in-js/sizes

commit 88709ac
Author: cchaos <[email protected]>
Date:   Mon Mar 8 16:46:31 2021 -0500

    Updating consuming example to show `calc()`

commit 60ef501
Author: Greg Thompson <[email protected]>
Date:   Wed Mar 3 17:46:29 2021 -0600

    fewer calc()

commit 10ad430
Author: Greg Thompson <[email protected]>
Date:   Wed Mar 3 12:37:09 2021 -0600

    docs

commit 34e0ba1
Merge: c897fa7 1b2f77b
Author: Greg Thompson <[email protected]>
Date:   Wed Mar 3 12:10:44 2021 -0600

    Merge branch feature/css-in-js into css-in-js/sizes

commit c897fa7
Merge: 9766b78 9074437
Author: Greg Thompson <[email protected]>
Date:   Mon Mar 1 11:08:17 2021 -0600

    Merge branch feature/css-in-js into css-in-js/sizes

commit 9766b78
Author: Greg Thompson <[email protected]>
Date:   Thu Feb 25 10:34:45 2021 -0600

    convert numbers to strings with units
… and anything conusming via `[COLOR_MODE_KEY]` should get converted to top level concern while anything just using `euiTheme.colors` would stay
Breaks test in components (??)
…ure/css-in-js_themes

# Conflicts:
#	src-docs/src/views/theme/consuming.tsx
#	src-docs/src/views/theme/consuming_hoc.tsx
#	src/services/theme/theme.ts
@cchaos cchaos marked this pull request as draft March 15, 2021 20:56
Copy link
Contributor Author

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@thompsongl We talked through a bunch of these, but thought it would be helpful to point them out in the code for easier reference too.

src-docs/src/components/with_theme/theme_context.tsx Outdated Show resolved Hide resolved
src-docs/src/views/emotion/canopy.tsx Outdated Show resolved Hide resolved
src-docs/src/views/emotion/canopy.tsx Outdated Show resolved Hide resolved
src/global_styling/functions/_typography.ts Outdated Show resolved Hide resolved
src/global_styling/variables/_typography.ts Outdated Show resolved Hide resolved
src/global_styling/variables/title.ts Outdated Show resolved Hide resolved
src/global_styling/variables/title.ts Outdated Show resolved Hide resolved
src/services/theme/theme.ts Outdated Show resolved Hide resolved
src/services/theme/theme.ts Outdated Show resolved Hide resolved
src/global_styling/variables/_typography.ts Outdated Show resolved Hide resolved
@cchaos cchaos requested a review from thompsongl March 15, 2021 21:11
@thompsongl

This comment has been minimized.

@thompsongl
Copy link
Contributor

See cchaos#44 for a concept on changing how colorMode keys work, and an update to allow dependecy-less computed methods.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@thompsongl
Copy link
Contributor

@cchaos Some unused variable files exist:

  • _colors_vis.ts
  • _z_index.ts
  • title.ts

Do you have any later plans for them or should we remove them?

@thompsongl
Copy link
Contributor

jenkins test this

@thompsongl thompsongl changed the title [DRAFT][CSS in JS] Global theme [CSS-in-JS] Global theme Jul 29, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@cchaos
Copy link
Contributor Author

cchaos commented Jul 29, 2021

Yeah they're not used yet but I didn't want to lose this initial conversion work. Im ok to leave in.

@thompsongl thompsongl marked this pull request as ready for review July 29, 2021 16:56
Copy link
Contributor Author

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🥳 I'm happy with and approve the state of these global items and the docs. 😉

The only other thing I'd like to improve is somehow getting all the <h2>s on the global values page into the side nav.
Screen Shot 2021-07-29 at 15 00 07 PM

But we can do a follow up for that.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4643/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

🙀

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cchaos cchaos merged commit 31a24ed into elastic:feature/css-in-js Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants