-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(Chip) replace with Label #621
feat(Chip) replace with Label #621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! 🔥 I like the idea of separating the fixer helper functions.
Couple of comments bellow, but mostly minor changes. The only blocking one is related to the Label / LabelGroup import - what if it is already imported, the user could end up with duplicate import specifier.
]; | ||
}; | ||
|
||
const getJSXAttribute = (node: JSXElement, attributeName: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be replaced with the getAttribute
helper.
const importedName = importSpecifier.imported.name; | ||
const name = importedName.replace(oldName, newName); | ||
const localName = importSpecifier.local.name; | ||
const aliasText = importedName !== localName ? ` as ${localName}` : ''; | ||
const rename = `${name}${aliasText}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these lines can be deleted as the rename
is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename should be used instead of newName
in the fixer, thanks for the catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can keep the newName
there as it can make things easier afterwards when renaming the JSXElement, so you don't have to check if an alias was used there. But it is up to you. Also the duplicate Label import might play a role, so it might end up being more complicated than I see it now.
return []; | ||
} | ||
|
||
const oldName = importToRename.imported.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can also be deleted.
const rename = `${name}${aliasText}`; | ||
return [ | ||
fixer.replaceText(importSpecifier, newName), | ||
fixer.replaceText(node.source, "'@patternfly/react-core'"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the replacement of the node.source
necessary?
message: `Chip has been deprecated. Running the fix flag will replace Chip and ChipGroup components with Label and LabelGroup components respectively.`, | ||
fix(fixer) { | ||
return [ | ||
...renameImportSpecifier(chipImport, node, fixer, 'Label'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user already has a Label or LabelGroup imported? We should cover that case too, so there is no name clashing of duplicate Label
imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts
Show resolved
Hide resolved
node.closingElement.name, | ||
'Label' | ||
), | ||
...moveBadgeAttributeToBody(node, fixer, context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that someone would use a Chip with Badge but without children (self-closing)? I am just wondering, whether this case should be covered:
<Chip onClick={onClick} badge={<Badge isRead={true}>7</Badge>} />
I guess probably not, or ... it would be too much work to handle it and it is not very likely.
@@ -0,0 +1,3 @@ | |||
import { Chip } from '@patternfly/react-core/deprecated'; | |||
|
|||
export const ChipReplaceWithLabelInput = () => <Chip onClick=(handleClick) badge={badge}></Chip>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was meant to be onClick={handleClick}
.
Also can we add some text as children of the Chip? (to better demonstrate the badge transformation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on this! A few small refactor nits and Adam's existing comments and I think this will be good!
packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-pf-codemods/src/rules/v6/chipReplaceWithLabel/chip-replace-with-label.ts
Outdated
Show resolved
Hide resolved
1bd422d
to
01ba164
Compare
- covers Label imports already used - covers multiple imports from deprecated package - modify nested JSXElements in one rule run
Did some changes to handle the situation with Also I discovered that when there were more elements nested, like this:
it would only modify the parent element on the first rule run (it eventually runs the rule 10 times, fixing the nested elements, but it is problematic with tests, where it only runs once) - due to some ranges clashing probably. Because of that and also to be able to properly remove the unused Chip and ChipGroup imports (otherwise after removing, we are not able to get the previous local name - alias of the import), it was necessary to apply all the fixes in one The only disadvantage is the consumers won't get errors for every line where the actual JSXElement is used, but only for the import declaration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
close #564