feat(Tabs): rename tabs props #205
Conversation
gitdallas
left a comment
gitdallas
left a comment
There was a problem hiding this comment.
just a couple little things
packages/eslint-plugin-pf-codemods/test/rules/v5/tabs-rename-props.js
Outdated
Show resolved
Hide resolved
| const renames = { | ||
| Tabs: { | ||
| hasSecondaryBorderBottom: "", | ||
| hasBorderBottom: "hasNoBorderBottom", |
There was a problem hiding this comment.
Should this codemod just rename the hasBorderBottom prop? @gitdallas would this be similar to your comment on the isSVG PR? Just wondering if simply renaming it might cause issues i.e. if someone has passed in hasBorderBottom={false} to remove the borders, would this then update things so that it becomes hasNoBorderBottom={false} and have borders rendered.
There was a problem hiding this comment.
good catch/point, the boolean value should be flipped. I think we can still do a code fix like below, wdyt? @thatblindgeye
hasBorderBottom => ""
hasBorderBottom={true} => ""
hasBorderBottom={false} => hasNoBorderBottom
hasBorderBottom={someVar} => hasNoBorderBottom{!someVar}
There was a problem hiding this comment.
That sounds good to me! I imagine that'd cover the basic use cases of it.
There was a problem hiding this comment.
started a slack convo regarding these more complex cases in patternfly#pf-devs but for example, the someVar could also be more complex so perhaps may need to be wrapped in () in case it is something like someVar || someOtherVar ... also trying to get a feel for if we should even be taking on these complex situations vs alerting the consumer and asking them to do it themselves.
jenny-s51
force-pushed
the
tabsBorderBottomProps
branch
3 times, most recently
from
91b8e0c to
636f7f8
Compare
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Great catch @thatblindgeye, this is certainly a good start to a larger discussion regarding other instances like this @gitdallas
I've updated this PR with changes that emulate what we did for the renaming of hasBodyPadding within the v4 Wizard component.
gitdallas
left a comment
gitdallas
left a comment
There was a problem hiding this comment.
this looks good and i've learned some from it. i wonder if we should also consider they might pass a variable or expression other than just true or false.
do you think the below change would handle that well?
packages/eslint-plugin-pf-codemods/lib/rules/v5/tabs-rename-hasBorderBottom.js
Outdated
Show resolved
Hide resolved
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
🎉 Looks nice! Just needs the conflicts resolved again
jenny-s51
force-pushed
the
tabsBorderBottomProps
branch
from
f50c88d to
7b39e0f
Compare
PR feedback, update output values update rule update readme add rule fix error msg fix error msg Dallas var names rule logic Co-authored-by: Dallas <dallas.nicol@gmail.com> PR feedback from Dallas, adding var names test
jenny-s51
force-pushed
the
tabsBorderBottomProps
branch
from
7b39e0f to
df7dedc
Compare
Closes #203