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

feat(Tabs): added warning that the type of Tabs children is now limited #220

Merged

Conversation

wise-king-sullyman
Copy link
Collaborator

Closes #168

Comment on lines -72 to -80

// Update index file
const ruleIndexPath = path.join(__dirname, 'packages/eslint-plugin-pf-codemods/index.js');
const ruleIndex = fs.readFileSync(ruleIndexPath, 'utf8');
fs.writeFileSync(
ruleIndexPath,
// (ab)Use fact that `rules` object is at top of file
ruleIndex.replace("};", ` "${newRuleName}": require('./lib/rules/v5/${newRuleName}'),\n};`)
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as the index file no longer needs to be manually updated with rule names.

if (TabsImport) {
context.report({
node,
message: "Children of the 'Tabs' component must now be 'Tab' components or expressions resulting in a falsy value.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this message, but it was the best I could come up with. 100% open to suggestions.

Copy link
Contributor

@gitdallas gitdallas Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk why but when i see "children of the" i think "corn" 😆

how about "Tabs component children must be Tab components"

maybe add "or falsy", not sure that the expressions part needs to be mentioned. technically it would be "expressions resulting in either falsy value or a Tab component"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The children prop of the Tabs component must now be passed a Tab component or a falsy value."?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thatblindgeye I like the sound of that, but since children isn't usually used explicitly as a prop I wonder if that might trip people up 🤔

@gitdallas so your suggestion would be "Tabs component children must be Tab components or expressions resulting in either falsy value or a Tab component" in its full form?

Copy link
Contributor

@gitdallas gitdallas Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wise-king-sullyman - i really don't think the mention of expressions is necessary, anything could be expression but that doesn't matter... all that matters is what comes out of the expression. if you believe it's important to mention expressions, lmk the thoughts behind that.

i was thinking "Tabs component children must be Tab components or a falsy value", I guess... or in Eric's format we could have "The children of the Tabs component must now be passed a Tab component or a falsy value."

@@ -0,0 +1,23 @@
const { getPackageImports } = require("../../helpers");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could remove this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@gitdallas gitdallas merged commit 6d8470a into patternfly:main Jan 25, 2023
@wise-king-sullyman wise-king-sullyman deleted the tabs-children-type-updated branch January 25, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabs - children prop type updated
3 participants