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

Proposal: variant prop #800

Closed
dburles opened this issue Mar 26, 2020 · 2 comments
Closed

Proposal: variant prop #800

dburles opened this issue Mar 26, 2020 · 2 comments

Comments

@dburles
Copy link
Contributor

dburles commented Mar 26, 2020

Expanding on #294 (and related to #706), I've been thinking about an idea of replacing the variant faux CSS property instead with a special variant prop.

As mentioned in #294, the variant API no longer becomes useful when dealing with components composed of multiple elements, this is a problem for both userland components (#294) and @theme-ui/components i.e. the Select component mentioned in #706.

Because of this, you are forced to abandon use of the variant API and instead implement something akin to what this variant prop proposal offers. The downside with that in the current implementation is then your component library will have two API's for using variants, (sx variant and variant prop), unless as I mentioned, you decide to implement a variant prop in your component library for all components that require it (as a workaround). With this proposal, that would not be required. e.g:

// Contrived example of the issue above
const Button = ({
  variant,
}) => {
  return (
    <button
      sx={{
        // ...
        variant: `buttons.Button.${variant}`,
      }}
    />
  )
};

In implementation of the variant prop, the example API @jxnblk mentioned in #706 could be altered as follows:

const theme = {
  ...
  forms: {
    select: {
      // styles for select element
      select: {
        borderRadius: 5,
      },
      container: {
        // styles for root element
      },
      arrow: {
        // styles for arrow icon go here
      },
    },
  },
}

The Select component from @theme-ui/components can now reference the variant prop (as now can any userland components too, and also easily pick from the theme object too if they wish):

Rough mockup:

export const Select = React.forwardRef((props, ref) => {
  const { theme } = useTheme();
  return (
    <Box
      {...getMargin(props)}
      sx={{
        display: 'flex',
        ...theme.forms.select.container,
      }}
    >
      <Box
        ref={ref}
        as="select"
        {...omitMargin(props)}
        __css={{
          display: 'block',
          width: '100%',
          p: 2,
          appearance: 'none',
          fontSize: 'inherit',
          lineHeight: 'inherit',
          border: '1px solid',
          borderRadius: 4,
          color: 'inherit',
          bg: 'transparent',
          ...theme.forms.select.select,
        }}
      />
      <DownArrow
        sx={{
          ml: -28,
          alignSelf: 'center',
          pointerEvents: 'none',
          ...theme.forms.select.arrow,
        }}
      />
    </Box>
  );
});

With some additional changes to the Select component, it would also be possible to support inline overrides, e.g:
This is probably not ideal as components with a single element would mean that variant={{ color: 'blue' }} would work too, but that would clash with the sx prop API. Better that the variant prop only extracts values from the theme.

<Select variant={{ arrow: { color: 'blue' } }}>...</Select>

Another change that will need to occur is that you can no longer use the variant property within the theme, but that can be solved by regular old object composition:

const input = {
  color: 'text'
};

const theme = {
  forms: {
    input,
    select: {
      // instead of variant: 'input',:
      ...input,
      // styles for select element
      borderRadius: 5,
      container: {
        // styles for root element
      },
      arrow: {
        // styles for arrow icon go here
      }
    }
  }
};

I'm definitely interested in any thoughts and ideas on this, I've spent some time thinking it through, however there may be some aspects that I've overlooked.

This was referenced Apr 4, 2020
@jxnblk jxnblk mentioned this issue Apr 7, 2020
16 tasks
@folz
Copy link
Contributor

folz commented Apr 9, 2020

Came across this issue when reading the v1 roadmap. I'd like to add support to the idea. Our design system uses theme-ui under the hood but already exposes a variant prop instead of using the variant css property. Specifically, we wanted to reinforce that sx is the low-level styling API and variant (the prop) is what to use for design-system-"blessed" styles.

There's also the benefit of removing the "css properties, also a 'variant' key" special case from sx.

@lachlanjc
Copy link
Member

Closing since https://github.com/dburles/mystical supports this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants