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

RFC: Level 2 Specification #5

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

RFC: Level 2 Specification #5

wants to merge 3 commits into from

Conversation

jxnblk
Copy link
Member

@jxnblk jxnblk commented Jun 28, 2019

This RFC (request for comments) is a proposal to add a second, optional specification that builds on top of the foundation of the current specification with the intent of making theme objects more portable than they currently are.

Rendered Text

@sapegin
Copy link

sapegin commented Jun 29, 2019

I have almost the same shape on my sites, but I use base instead of body:

{
   fontWeights: {
	base: 300,
	heading: 300,
  },
  lineHeights: {
	base: 1.5,
	heading: 1.1,
  },
  letterSpacings: {
	base: 0,
	heading: 0,
  }
}

@felipeleusin
Copy link

In my team we had some strong feelings against using array of numbers and opted out with object approach for things like sizes, breakpoints. This would be a break from level 2 correct?

While I get the appeal of the array pattern and doing something like paddingTop: 2 we ended up deciding with with paddingTop: sm.

But I really like the idea of a pre-defined flat structure for colors and other things, specially since we're using TypeScript a lot being able to do something like:

type ThemeColors = keyof Pick<ThemeObject,'colors'>

really helped us with discovery and onboarding new folks

@arronhunt
Copy link

arronhunt commented Aug 23, 2019

colors: {
    text: String,
    background: String,
    primary: String,
    secondary: String,
    accent: String,
    muted: String,
},

Something that is notably missing here are the relationships between text colors and their different backgrounds. In reality you never have a single "text" color; there is a tight relationship between the color of a text node and the background color of its parent (for example a primary action button).

Here's a sample of what code would look like for designing a blue primary button using styled-components.

theme.colors.text = '#111111';
theme.colors.primary = 'blue';

const BaseButton = styled.button`
  color: ${({ theme }) => theme.colors.text };
  /* other styles */
`;

const PrimaryButton = styled(BaseButton)`
  background-color: ${({ theme }) => theme.colors.primary };
  color: #ffffff; /* Nothing in my theme that gives me the proper text color */
`;

This of course can get very hairy fast. A beginner's first instinct may be to define a 'dark' and 'light' text color and then select the appropriate one based on the background, however that doesn't give us enough control and really locks us down in terms of styling buttons etc.

Competing specs mentioned in Prior Art solve for this but are also overly complex. The benefits I'm seeing from this project are simplicity. I don't have the right answer here so I'd love to see thoughts from others. Here's a potential solution that is relatively "simple".

colors: {
  primary: {
    text: String;
    background: String;
  }
  // ...etc
}

@alaboudi
Copy link

on the same note to that mentioned by @arronhunt , I tend to use primary and onPrimary in my theme spec when defining my colors.

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.

5 participants