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

Create a lint rule that enforces appropriate usage of Box #55

Open
2 tasks
khiga8 opened this issue Jun 6, 2023 · 4 comments
Open
2 tasks

Create a lint rule that enforces appropriate usage of Box #55

khiga8 opened this issue Jun 6, 2023 · 4 comments
Assignees
Labels

Comments

@khiga8
Copy link
Contributor

khiga8 commented Jun 6, 2023

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.

Related slack thread

Acceptance criteria

  • Propose a lint rule that encourages appropriate usage of Box and/or auto-fixes it.
  • Incorporate auto-fix.
@lesliecdubs
Copy link
Member

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.

@inkblotty inkblotty changed the title Create a lint rule that suggests a more appropriate component [Spike] Create a lint rule that suggests a more appropriate component Jul 5, 2023
@inkblotty inkblotty changed the title [Spike] Create a lint rule that suggests a more appropriate component [Spike] Create a lint rule that suggests a more appropriate component for as Jul 5, 2023
@khiga8 khiga8 changed the title [Spike] Create a lint rule that suggests a more appropriate component for as [Spike] Create a lint rule that suggests a more appropriate component for as for Box May 15, 2024
@khiga8 khiga8 changed the title [Spike] Create a lint rule that suggests a more appropriate component for as for Box Create a lint rule that suggests a more appropriate component for as for Box May 16, 2024
@khiga8
Copy link
Contributor Author

khiga8 commented May 16, 2024

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.

Related slack thread

@khiga8 khiga8 changed the title Create a lint rule that suggests a more appropriate component for as for Box Create a lint rule that enforces appropriate usage of Box May 16, 2024
@khiga8 khiga8 self-assigned this May 16, 2024
@khiga8
Copy link
Contributor Author

khiga8 commented May 16, 2024

Additional questions around validity of <Heading> and <Text>.

@khiga8
Copy link
Contributor Author

khiga8 commented May 21, 2024

@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.

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

No branches or pull requests

3 participants
@lesliecdubs @khiga8 and others