feat(Button): Extend icon moving to support Icon and react-icon children #735
Conversation
There was a problem hiding this comment.
Couple of comments bellow, also there is a problem that this rule removes all children of the Button (because it was assuming only the plain button variant before), which becomes destructive in case like this:
import { Button } from "@patternfly/react-core";
import { SomeIcon } from "@patternfly/react-icons";
<Button>
Text
<SomeIcon />
</Button>
where the "Text" will be removed.
| if (childrenProp) { | ||
| const childrenPropExpression = getExpression( | ||
| childrenProp?.value | ||
| ); | ||
| childrenValue = childrenPropExpression | ||
| ? context.getSourceCode().getText(childrenPropExpression) | ||
| ? source.getText(childrenPropExpression) |
There was a problem hiding this comment.
| ? source.getText(childrenPropExpression) | |
| ? `{${source.getText(childrenPropExpression)}}` |
Not related to a change in this PR, but we should wrap this in an expression bracket, otherwise this: <Button variant="plain" children={<span>Some icon</span>} /> would result to an icon argument not being wrapped: <Button icon=<span>Some icon</span> variant="plain" />
| type: "JSXElement", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Can we also add a test for the children attribute mentioned above?
| }, | |
| }, | |
| { | |
| code: `import { Button } from '@patternfly/react-core'; <Button variant="plain" children={<span>Some icon</span>} />`, | |
| output: `import { Button } from '@patternfly/react-core'; <Button icon={<span>Some icon</span>} variant="plain" />`, | |
| errors: [ | |
| { | |
| message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`, | |
| type: "JSXElement", | |
| }, | |
| ], | |
| }, |
| return; | ||
| } | ||
|
|
||
| const iconComponentChild = getChildElementByName(node, "Icon"); |
There was a problem hiding this comment.
I think we should check an import { Icon } from react-core, otherwise this will get any element named "Icon".
| @@ -1,7 +1,18 @@ | |||
| import { Button } from "@patternfly/react-core"; | |||
There was a problem hiding this comment.
Related to the comment above, an Icon import should be included here in order for the second element to have the rule applied.
| isReactIcon(context, child) | ||
| ); | ||
|
|
||
| const iconChild = iconComponentChild || reactIconChild; |
There was a problem hiding this comment.
I wonder if we should also handle this icon child in case of children as an attribute. Because currently this is valid:
import { Button } from '@patternfly/react-core';
import { SomeIcon } from "@patternfly/react-icons";
<Button children={<SomeIcon />} />
and the rule won't be applied.
I think it is a pretty rare use case and it would complicate the rule, so I wouldn't bother.
| @@ -1,7 +1,18 @@ | |||
| import { Button } from "@patternfly/react-core"; | |||
There was a problem hiding this comment.
| import { Button } from "@patternfly/react-core"; | |
| import { Button, Icon } from "@patternfly/react-core"; |
| @@ -1,5 +1,12 @@ | |||
| import { Button } from "@patternfly/react-core"; | |||
There was a problem hiding this comment.
| import { Button } from "@patternfly/react-core"; | |
| import { Button, Icon } from "@patternfly/react-core"; |
There was a problem hiding this comment.
Ah yep, great catches!
Closes #716