Eslint plugin: prefer-box-no-classname rule w/ autofix + merged into prefer-box-lonely-ref
#1629
Conversation
AlbertCarreras
changed the title
prefer-box-no-classname rule w/ autofix + merged into prefer-box-lonely-ref
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
from
1b4f78a
to
be55a1c
Compare
|
✔️ Deploy Preview for gestalt ready! 🔨 Explore the source changes: 1b4f78a 🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/611b080da27ac6000869ce5c 😎 Browse the preview: https://deploy-preview-1629--gestalt.netlify.app/ |
|
✔️ Deploy Preview for gestalt ready! 🔨 Explore the source changes: d293baf 🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/611ee012a1c71700084b4e20 😎 Browse the preview: https://deploy-preview-1629--gestalt.netlify.app/ |
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
2 times, most recently
from
556fc2c
to
8e90f77
Compare
| newComponentName, | ||
| replaceRegexCallback = ({ input }) => input, |
There was a problem hiding this comment.
This seems a bit specific to dangerouslySetInlineStyle. I wonder if it would make more sense to build that prop (and any others) as a string outside of this function, then use RenameTagWithPropsFixerType's additionalPropsString arg to build it into the fixed component?
There was a problem hiding this comment.
This is the less "invasive" approach, I'm not just renaming, but adding the { __style: <>} first key value . Adding one additional props would require removing the previous style prop. I could also reconstruct all props and alphabetize, but that would make the PR review even more opaque to the important changes. I might change the approach when I work on other Eslints, there must be a more universal way. For now, I think I wanna keep this one.
There was a problem hiding this comment.
What do you think of the approach I took with the Box->Flex rule?
Avoiding regex keeps the code more readable, and it seems a lot simpler to me to keep the rule-specific logic outside of these helpers. The helpers can remove a current prop or add a new one, and the logic within the rule can determine what needs to be removed and what needs to be added (and how the new prop(s) should be formatted).
There was a problem hiding this comment.
Nice! Looks great. This arg can now be removed, right?
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
from
f2649f2
to
5b51b27
Compare
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
2 times, most recently
from
01b2ba6
to
6ec8685
Compare
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
from
6ec8685
to
ba913f4
Compare
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
from
ba913f4
to
dfca97e
Compare
| @@ -160,7 +181,7 @@ type GetComponentFromAttributeType = ({| nodeAttribute: GenericNode |}) => Gener | |||
| Example: | |||
| \<div {...props} \/\> returns div node for the spread props attribute | |||
| */ | |||
| export const getComponenFromAttribute: GetComponentFromAttributeType = ({ nodeAttribute }) => | |||
| const getComponenFromAttribute: GetComponentFromAttributeType = ({ nodeAttribute }) => | |||
There was a problem hiding this comment.
nit: typo, getComponentFromAttribute
| const isTag: IsTagType = ({ elementNode, tagName }) => elementNode?.name?.name === tagName; | ||
|
|
||
| type HasSpreadAttributesType = ({| elementNode: GenericNode |}) => boolean; | ||
| /** This function checks is a given node (elementNode) contains spread attributs |
There was a problem hiding this comment.
nit: typo attributes
| attributes: $ReadOnlyArray<string>, | ||
| |}) => boolean; | ||
|
|
||
| /** This function checks is a given tag (tagName) in a node (elementNode) contains a given attribute (attribute), and returns true if so. |
There was a problem hiding this comment.
nit: typo …checks if a given…
| newComponentName, | ||
| replaceRegexCallback = ({ input }) => input, |
There was a problem hiding this comment.
Nice! Looks great. This arg can now be removed, right?
|
Yeah, I left it because I thought we could use it in the future, preoptimization... so not need it. |
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
2 times, most recently
from
4dcf395
to
35692f2
Compare
AlbertCarreras
force-pushed
the
preferBoxNoClassname
branch
from
35692f2
to
d293baf
Compare
Summary
prefer-box-no-disallowed: "Prefer Box: prevent
<div>tags that don't contain disallowed attributes:className,onClick. Use Gestalt Box, instead"This is a list of the eslint-plugin-jsx-a11y eslints that cover div but won't cover Box. Other props [
role,onMouseOver,onMouseOut,accessKey,autoFocus,tabIndex] are also considered ignored or disallowed to prevent losing coverage:Good to go on Pinboard:
`TIMING=1 ./node_modules/.bin/eslint ~/code/pinboard/webapp/app
No errors thrown.
Example:
Links
Checklist