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

docs(variants): Clearly explain variant notation #1494

Merged
merged 2 commits into from Feb 15, 2021

Conversation

lachlanjc
Copy link
Member

At @erikdstock’s excellent suggestion, clarifying how the notation for accessing variants works.

@lachlanjc lachlanjc added the affects-docs Changes affect documentation, but not only documentation label Feb 8, 2021
@vercel
Copy link

vercel bot commented Feb 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/fpgy38nht
✅ Preview: https://theme-ui-git-docs-variant-notation.systemui.vercel.app

@erikdstock
Copy link

erikdstock commented Feb 9, 2021

Looks great! I'm not sure how useful any of this would be useful for documentation but I'd like to clarify - styles are applied and overridden in this order:

  1. theme.styles.root: automatically applied from the body element
  2. theme.styles.<tagName> for MDX or <Themed[tagName] / > or Styled.[tagname]` components
  3. variants (probably best not to mix them):
  • built-in variant prop for theme-ui/componentssx eg <Button variant="outline"
  • sx prop variant key with full path eg <div sx={{variant: "buttons.outline"}}
  1. sx prop styles
  2. (maybe an anti-pattern) className manually passed down from custom parent components, eg
const CustomButton = ({ children, className }) => (
  <Button
    className={className}
    variant="primary"
    sx={{ borderColor: "purple" }}
  >
    {children} (but custom)
  </Button>
)

// sx is converted to className
<CustomButton sx={{p: 5}}>Big Button</ CustomButton>

Finally, within the theme-ui/components: variant prop, which may have its own special key/default (eg theme.buttons), using a full theme path with the dot is an option as well- eg <Button variant="secondary" is equivalent to variant="buttons.secondary" and <Button variant="cards.primary" or variant="styles.button" would also be permitted, if not advisable.

Example codesandbox

I'm also wondering if there are any identified anti-patterns - In the example above radical departures like adding link styles to a button don't seem to work, and I've also found that there is always a balance to strike as far as what belongs isolated in a theme file vs declared where it is used or where some defaults are applied.

Another thought i'd be willing to take on - it would be nice to add doc comments for default theme keys to the custom components.

Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

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

Great improvement, folks.

I'm approving both the current addition and any changes / more content based on Erik's comment.

@lachlanjc
Copy link
Member Author

Thanks @erikdstock! I added a section about styles linking to various places those are used. I’m going to merge this now as I don’t want making this perfect to interfere with getting something better live, but we could definitely keep making this better. Appreciate the feedback as always.

@lachlanjc lachlanjc merged commit 3a67dbc into develop Feb 15, 2021
@lachlanjc lachlanjc deleted the docs-variant-notation branch February 15, 2021 01:54
Variants inside the `styles` object, while usable through the same mechanisms as regular variants,
are also used for other Theme UI APIs.

- The entire [`styles` object](/theme-spec#styles) can style child HTML elements or MDX content,
Copy link
Member

Choose a reason for hiding this comment

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

Hey @lachlanjc, I'm not sure if this bullet doesn't "conflict" with the last one.
BaseStyles is used to style embedded HTML content, but MDX is styled with Themed (i.e. ## maps to Themed.h2)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Using Next.js that's not the case (MDX is treated the same as HTML children, so you need BaseStyles), so I've never experienced that. I guess we can just remove the MDX reference for now?

Copy link
Member

@hasparus hasparus Feb 15, 2021

Choose a reason for hiding this comment

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

MDX is treated the same as HTML children

That MDXProvider rendered by our ThemeProvider doesn't work in Next? (We pass an object with components through context)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope: #1130

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't a new version of MDX packages specified in resolutions solve that issue?

#1130 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps! Forgot about that & have never tried it

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can just remove the MDX reference for now?

Yeah, this sound like a good idea :D

Copy link
Member Author

Choose a reason for hiding this comment

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

@erikdstock
Copy link

Thanks yall, this is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-docs Changes affect documentation, but not only documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants