Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Enforce usage of the theme #440

Closed
Grsmto opened this issue Nov 6, 2019 · 11 comments
Closed

Enforce usage of the theme #440

Grsmto opened this issue Nov 6, 2019 · 11 comments
Labels
enhancement New feature or request

Comments

@Grsmto
Copy link

Grsmto commented Nov 6, 2019

Hi 😃

After using ThemeUI extensively for various websites and Styled-System as the core for a component library of a design system, I experienced issues with developer experience not following the constraint-based pattern:
Often developers end up hard coding the values instead of using the theme.

I know it's a problem of discoverability and documentation but I was thinking that there should also be a better way to make sure the developers know what they are doing.

So I see these options:

  • A linter (Eslint plugin) that would warn user if the value of a theme-aware css property is hardcoded. User would then have to add an inline comment to force it (as anyway it should happen a very few places in an app).
  • Enforce it at runtime with an error/warning. But I don't see how user could opt-out to that contextually.
  • Be something part of the VSCode extension (VS Code plugin #91)

What do you guys think?

@joestrouth1
Copy link
Contributor

This is an interesting idea that I see a few issues with:

  • sx={{ color: 'red' }} could be a theme value, or a 'hardcoded' CSS keyword depending on the theme. The linter would need to know about the theme(s) in the current context in order to decide which. That information may not be available.
  • Design systems vary widely in strictness. What may happen very few times in one app may be relatively common in another. Even within one company, the strictness may vary by context. Consider sharing components & theme between a core product/admin area and marketing/landing pages.

@folz
Copy link
Contributor

folz commented Nov 9, 2019

One possible approach (that I have not actually done yet, but it's on my roadmap) is to use a type system in conjunction with tagged literals to enforce theme values while allowing opt-out. My plan is:

  1. Write a raw tagged template function which returns an opaque type Raw mapping to string.
  2. Update typings for the sx prop to always reference theme values or Raw, instead of allowing a generic string as fallback.
  3. Have any linters, etc, check for usages of raw to discover non-theme values.

From a developer experience perspective, it would look something like:

<MyComponent sx={{ color: 'red', backgroundColor: raw`gray` }} />

Where 'red' is theme.colors.red, and 'gray' is the literal raw value 'gray'.''

This way, the type system can enforce usage of the theme while still providing an opt-out when necessary. It gives you the ability to discover where engineers need to break out of the theme, but doesn't otherwise block that usage.

Here's an example: Try Flow.

Typescript doesn't quite have a concept of opaque types, but there are discussions about similar things in this issue: microsoft/TypeScript#202

If you do decide to go this route, I'd love to know how it works out!

@peduarte
Copy link
Contributor

peduarte commented Mar 9, 2020

I like the idea of a raw function. I remember having a chat with @jxnblk about this a long time ago.

Still think it'd be a good idea to default to theme, and explicitly unlink from the theme when needed.


Here's a common case for me:

The main reason for me is with the lineHeight CSS Property. Here's why:

Imagine you have the following theme:

const theme = {
  lineHeights: [15, 20, 25, 35]
};

And you want to set the following CSS declaration on an element:

line-height: 1;

Note, we want to use a unitless line height here

Typically, you'd do:

<Element sx={{ lineHeight: '1' }} />

But that returns the following declaration:

line-height: 15px;

Technically speaking, this is the correct behaviour, given the current implementation, but how does one achieve the desired goal?

Since the sx prop supports functional values I thought the following would work:

<Element sx={{ lineHeight: () => '1' }} />

But no dice.

@cour64
Copy link

cour64 commented Mar 9, 2020

@peduarte Hmm that's odd, the line heights are working fine for me here

@mxstbr is working on something similar named 'strict-ui' see this PR

I really like the sound of a raw tagged template literal to enforce usage of them theme

@folz
Copy link
Contributor

folz commented Mar 9, 2020

@peduarte We've moved away from array indexing in favor of named tokens for all design system values, to avoid precisely that problem. That is,

const theme = {
  space: {
    small: 2,
    medium: 4,
  }
};

<Box sx={{p: 'small'}} />

and not

const theme = {
  space: [0, 2, 4, ...]
}

<Box sx={{p: 1}}

Feedback from engineers was overwhelmingly that array-based scales were confusing because they conflated literal values with array indexes, so you had to memorize the length of each scale or look it up each time. By switching to token keys, we avoided this problem.

@peduarte
Copy link
Contributor

peduarte commented Mar 9, 2020

@cour64 Your demo has the issue I described. The computed line height of your heading is 20, when I would like it to be 1

image


@folz I feel you man, that's fair. Would love to know more about your experience with your engineers using this API 😄

@mxstbr
Copy link
Member

mxstbr commented Mar 9, 2020

Quick note regarding the plans for the strict-ui PR (#719): I'm just experimenting around with some ideas, it is very possible that the entire thing will not go anywhere.

That being said, we will very likely integrate the "only use tokens from the theme"-validation into a "theme-ui strict mode" (potentially @theme-ui/strict but we're not sure about the specifics yet).


Regarding the escape hatch from only using theme values (e.g. the raw tagged template above): I don't think there should be an escape hatch of any kind. I think if you're using strict mode and you want to use a new value, you should add it to the theme.

Yes it will be ugly ("we already have lightgrey and grey, but we want another light grey, what do we call it?! mediumlightgrey??") but that's the point: it forces you to consider how the new value works within the overall system. If you really want to use a new value you have to add it, potentially under an ugly name—then at least it's obvious that the system is "tainted" and you can eventually work on remedying it.

@folz
Copy link
Contributor

folz commented Mar 10, 2020

Yeah, that's a fair position to take.

We're still formalizing our design system at Nextdoor. I don't want to block potentially legitimate usage of things we haven't included yet, which is why I came up with the raw idea for an escape hatch. I also think that escape hatches are valuable in general - for experimentation, and because I think there can always be a legitimate business reason to do things outside the design system (or put another way, before the design system catches up to the use case), but that doesn't mean theme-ui has to support it.

The raw idea above also doesn't really solve for the issue of platform-intrinsic names conflicting with theme token names - for example, if your design system uses 'goldenrod' as a color token, but you really want to use the CSS color 'goldenrod' instead. Our decision to use named tokens and not array-based scales helps avoid conflicts with number literal values, but if an engineer really wanted to use 'goldenrod'-the-CSS-color, they currently would have to look up the hex value and use that instead of the color name.

@mxstbr
Copy link
Member

mxstbr commented Mar 10, 2020

We're still formalizing our design system at Nextdoor. I don't want to block potentially legitimate usage of things we haven't included yet, which is why I came up with the raw idea for an escape hatch.

This is interesting feedback! In all design systems I have seen user-level customization of the overall "theme" was possible. Is that not the case with yours?

For example, the ThemeProvider pattern is very common and allows extending the overall theme as an escape hatch for random values:

// company-some-app/index.js
import { theme } from '@company/design-system';

<ThemeProvider
  theme={{
    ...theme,
    colors: {
      ...theme.colors,
      darkmediumlightgrey: '#FAFAFA' // NOTE: we should talk to the ds team about including this in the actual system based on why we need it
    }
  }}
>
  <App />
</ThemeProvider>

@folz
Copy link
Contributor

folz commented Mar 11, 2020

We do use ThemeProvider but it's not directly exposed as a public-level API. It's not deliberate - the idea of supporting that sort of userland customization didn't occur to me. But supporting that makes sense, and would help teams rely more on sx overall. Thanks for the idea.

I think I'm in favor of it generally, but can think of four situations that might lead to issues:

  1. Accidental or deliberate value overrides to existing design token values via userland overrides.
  2. If we update the theme to actually include a darkmediumlightgray token and miss updating a callsite which uses a different value for that token.
  3. Having token names on one platform (web) which don't exist on other platforms (iOS, Android).
  4. A component assumes a userland token exists, but is included (say, by another team), in a tree without that userland token in the provider.

All of these are more social problems and not really technical ones, though. Perhaps having a vendor-prefix style convention for userland overrides, like _darkmediumlightgray, would avoid those naming conflicts and help show where teams need to override.

Do you, or anyone else in this thread, have thoughts on these possible issues and how (or if) to solve for them?

@Peeja
Copy link

Peeja commented May 4, 2020

@mxstbr

Regarding the escape hatch from only using theme values (e.g. the raw tagged template above): I don't think there should be an escape hatch of any kind. I think if you're using strict mode and you want to use a new value, you should add it to the theme.

At CircleCI, we do a lot of experimentation, so we want to be able to cheaply build things that may never go anywhere. Ideally those things fit our current design tokens, and we're leveraging our existing theme and components to build higher-level ideas, but when there's conflict that's not easy to resolve, we want to test the viability of the design we're implementing and validate that it's worth pursuing for real before we change our theme to accommodate it. That's where an escape hatch is useful. Similarly, I never want to use any in TypeScript, but sometimes it's the practical thing to do, at least temporarily; no sense in going deep on a type issue over a feature that's still liable to get thrown out in a week's time.

@lachlanjc lachlanjc added the enhancement New feature or request label Dec 3, 2020
@system-ui system-ui locked and limited conversation to collaborators Jan 17, 2022
@lachlanjc lachlanjc converted this issue into discussion #2083 Jan 17, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants