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

Standardize common component props #1423

Open
scurker opened this issue Mar 20, 2024 · 0 comments
Open

Standardize common component props #1423

scurker opened this issue Mar 20, 2024 · 0 comments
Labels
breaking enhancement New feature or request rfc An issue proposing a new significant change
Milestone

Comments

@scurker
Copy link
Member

scurker commented Mar 20, 2024

We have a number of component props that differ between their usage and aren't consistent, examples below:

Refs

There's a mix of [componentName]Ref and ref. Generally speaking, every component should be using React.forwardRef and standardize on allowing ref. This is already part of of our component guidelines so new components should already be adhering to this:

Primitive/Pattern components should always use React.forwardRef to provide direct access to the component's primary element

However, we need to do the work to remove existing props (likely as a future breaking change) to standardize on ref.

Sizing

There's a mix of "sizing" props to allow for different "sizes" of components. This differs from variant as variant tends to be purely aesthetic. Some examples:

  • <Button thin>
  • <IconButton large>
  • <Link thin>
  • <Pagination thin>
  • <Tabs thin>
  • <Tooltip variant="big">

For the most part we've been using thin to indicate a more lightweight variant, but that hasn't always worked where the smaller version was already the (e.g. IconButton). The current methodology of using thin may be limiting in the future if we need to add additional sizes later considering we're treating these sizing props as boolean values. We would not want to allow <Button thin large> as that would not be a valid combination of props.

Instead, we should migrate over to using size to allow for greater flexibility in the future. This would allow us to better constrain our types as size="small" size="large" would be much easier to handle vs <Button thin large>.

Shared Component Props

When a component is "composed", we should have a scalable way of specifying props on the composed component. Some examples:

  • <IconButton tooltipPlacement={...}>
  • <Pagination tooltipPlacement={...}>
  • <TextEllipsis tooltipProps={...}>

The primary issue with the specifying individual custom props is if additional props are added to the underlying component or if certain props aren't included, it makes the composed component less flexible. In some cases this may be the desired behavior, but a nested component that receives props via composition should generally have a single prop on the composed component ideally with the pattern [componentName]Props.

@scurker scurker added enhancement New feature or request breaking rfc An issue proposing a new significant change labels Mar 20, 2024
@scurker scurker added this to the Future milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request rfc An issue proposing a new significant change
Projects
None yet
Development

No branches or pull requests

1 participant