Create a lint rule that enforces appropriate usage of `Box`
Opened this issue · 4 comments
Problem statement
Primer React provides a couple of low-level utility components such as <Box>, <Text>, and <Heading>.
In particular, <Box> seems to be used in all sorts of ways, outside of what the doc describes:
basic wrapper component for most layout related needs.
For example, there's <Box as="h1">, <Box as="button">, when people could be rendering <Heading> or <h1> , or <Button> instead. Box usage outside of "wrapper-y" elements (e.g. div, section) feel like code smells.
There is an opportunity to create a linter that suggests a more appropriate element or component based on the as prop.
This isn't directly tied to accessibility, but components being used in a predictable manner is important.
<Box> rendered without sx are unnecessarily verbose and might as well just be semantic HTML elements. This will not only improve readability, but it would also apparently improve performance.
Acceptance criteria
- Propose a lint rule that encourages appropriate usage of
Boxand/or auto-fixes it. - Incorporate auto-fix.
Thanks for the suggestion @khiga8! This seems like a good idea for a component like Box, but may be tricky to implement more broadly since as is sometimes used properly to enforce better semantics. We'll try to take a look at this at some point for Box if nothing else.
Another thing that I noticed is that there's a ton of usage of Box like:
<Box as="li"></Box>when one could simply render <li>. These could probably be auto-fixed to use HTML elements.
It appears this serves no utility, makes code harder to read, and additionally contributes to performance issues.
Additional questions around validity of <Heading> and <Text>.
@langermank brought up that there's an opportunity to flag migration from Box to the new Stack.
So far, there's quite a few "practices" we can enforce with Box which I'll note for now:
<Box>without sx usage should just be semantic HTML elements… this seems autofixable.<Box as=>set to anything interactive or with alternate Primer components shouldn’t use Box .. they are code smells
*<Box sx={ display: 'flex'>is a candidate for Stack. These might be autofixable too, depending on what other props are present.
These probably should be separate rules.