Adds a no-system-props rule #12
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 0555c31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
colebemis
reviewed
Oct 1, 2021
src/rules/no-system-props.js
Outdated
| }, {}) | ||
|
|
||
| // Create an array of bad prop attribute nodes | ||
| let badProps = Object.values(pick(propsByNameObject)) |
There was a problem hiding this comment.
How do you feel about calling this variable systemProps instead of badProps? I think that would align nicely with the name of the rule (no-system-props)
src/rules/no-system-props.js
Outdated
Comment on lines
39
to
43
| const propsByNameObject = jsxNode.attributes.reduce((object, cur) => { | ||
| // We don't do anything about spreads for now — only named attributes: | ||
| if (cur.type === 'JSXAttribute') { | ||
| object[cur.name.name] = cur | ||
| } |
There was a problem hiding this comment.
Calling this attribute instead of cur might be a litte more clear:
Suggested change
| const propsByNameObject = jsxNode.attributes.reduce((object, cur) => { | |
| // We don't do anything about spreads for now — only named attributes: | |
| if (cur.type === 'JSXAttribute') { | |
| object[cur.name.name] = cur | |
| } | |
| const propsByNameObject = jsxNode.attributes.reduce((object, attribute) => { | |
| // We don't do anything about spreads for now — only named attributes: | |
| if (attribute.type === 'JSXAttribute') { | |
| object[attribute.name.name] = attribute | |
| } |
src/rules/no-system-props.js
Outdated
| fixable: 'code', | ||
| schema: [], | ||
| messages: { | ||
| noSystemProps: 'Styled-system props are deprecated ({{componentName}} called with props: {{ propNames }})' |
There was a problem hiding this comment.
Suggested change
| noSystemProps: 'Styled-system props are deprecated ({{componentName}} called with props: {{ propNames }})' | |
| noSystemProps: 'Styled-system props are deprecated ({{ componentName }} called with props: {{ propNames }})' |
src/rules/no-system-props.js
Outdated
| ...(stylesToAdd.size > 0 | ||
| ? [ | ||
| existingSxProp | ||
| ? // Update an existing sx prop: |
There was a problem hiding this comment.
This is super nitpicky (so feel free to ignore): Is there a reason some of your comments end with a colon and some don't?
src/utils/getVariableDeclaration.js
Outdated
| @@ -0,0 +1,17 @@ | |||
| /** | |||
There was a problem hiding this comment.
Unless you have strong opinions otherwise, can we name these utility files with kebab case instead of camel case to be consistent with the rest of the filenames in this repo?
docs/rules/no-system-props.md
Outdated
| @@ -0,0 +1,38 @@ | |||
| # Disallow use of style system props (no-system-colors) | |||
There was a problem hiding this comment.
Suggested change
| # Disallow use of style system props (no-system-colors) | |
| # Disallow use of styled-system props (no-system-colors) |
docs/rules/no-system-props.md
Outdated
|
|
||
| 🔧 The `--fix` option on the [ESLint CLI](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule. | ||
|
|
||
| System props are deprecated in Primer components (excluding utility components). |
There was a problem hiding this comment.
Suggested change
| System props are deprecated in Primer components (excluding utility components). | |
| [Styled-system](https://styled-system.com/table) props are deprecated in Primer React components (excluding utility components). |
There was a problem hiding this comment.
Can we also mention that the sx prop is now the preferred method of applying custom styles?
src/rules/no-system-props.js
Outdated
Comment on lines
16
to
17
| ['Dialog', new Set(['width', 'height'])], | ||
| ['Label', new Set(['variant'])], |
There was a problem hiding this comment.
Flash also accepts a variant prop:
Suggested change
| ['Dialog', new Set(['width', 'height'])], | |
| ['Label', new Set(['variant'])], | |
| ['Dialog', new Set(['width', 'height'])], | |
| ['Flash', new Set(['variant'])], | |
| ['Label', new Set(['variant'])], |
docs/rules/no-system-props.md
Outdated
Comment on lines
25
to
38
| ```jsx | ||
| /* eslint primer-react/no-system-props: "error" */ | ||
| import {Box, Button, ProgressBar} from '@primer/components' | ||
| import {Avatar} from 'some-other-library' | ||
|
|
||
| <Button sx={{width: 200}} />, | ||
| <Button someOtherProp="foo" />, | ||
|
|
||
| <ProgressBar bg="howdy" /> | ||
|
|
||
| <Box width={200} />, | ||
|
|
||
| <Avatar width={200} />, | ||
| ``` |
There was a problem hiding this comment.
A couple of notes here:
- I think a prop like
mr={}is more recognizable as a styled-system prop thanwidth={}. Usingmr={}as our example in this document might make help people understand the idea faster. - Adding comments about why these examples are correct could be helpful.
Suggested change
| ```jsx | |
| /* eslint primer-react/no-system-props: "error" */ | |
| import {Box, Button, ProgressBar} from '@primer/components' | |
| import {Avatar} from 'some-other-library' | |
| <Button sx={{width: 200}} />, | |
| <Button someOtherProp="foo" />, | |
| <ProgressBar bg="howdy" /> | |
| <Box width={200} />, | |
| <Avatar width={200} />, | |
| ``` | |
| ```jsx | |
| /* eslint primer-react/no-system-props: "error" */ | |
| import {Box, Button, ProgressBar} from '@primer/components' | |
| import {Avatar} from 'some-other-library' | |
| // Non-system props are allowed | |
| <Button someOtherProp="foo" /> | |
| // If you need to override styles, use the `sx` prop instead of system props | |
| <Button sx={{mr: 2}} /> | |
| // Some component prop names overlap with styled-system prop names. | |
| // These props are still allowed | |
| <ProgressBar bg="success.emphasis" /> | |
| // Utility components like Box and Text still accept system props | |
| <Box mr={2} /> | |
| // System props passed to non-Primer components are allowed | |
| <Avatar mr={2} /> |
docs/rules/no-system-props.md
Outdated
|
|
||
| ## Rule details | ||
|
|
||
| This rule disallows use of any styled system prop on a Primer component. |
There was a problem hiding this comment.
Suggested change
| This rule disallows use of any styled system prop on a Primer component. | |
| This rule disallows the use of any styled-system prop on Primer React components. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.