feat(TreeView): warning for removal of selectable styling modifier #617
Conversation
| const treeViewImport = imports.find( | ||
| (specifier) => specifier.imported.name === "TreeView" | ||
| ); | ||
|
|
There was a problem hiding this comment.
Could we add some logic so that it only throws this warning in the event that pf-m-selectable would've been applied?
For easy reference that logic was children && (isSelectable || hasCheckbox)
There was a problem hiding this comment.
After doing some investigation, we are not able to check it and we will have to throw a warning every time.
The reason is simple: the children and hasCheckbox props depend on the individual TreeViewDataItem objects from data: TreeViewDataItem[] prop of the TreeViewProps.
And since the TreeView may be used like this:
import {options} from "somewhere";
<TreeView data={options} ...
we are not able to trace it and check the condition.
There was a problem hiding this comment.
Ah, yeah, good point. Since it will just be a warning it's better that we err on the side of showing the warning when it may not be needed than not showing it when it is.
| context.report({ | ||
| node, | ||
| message: | ||
| "Selectable styling attribute (`pf-m-selectable`) has been removed in TreeView component.", |
There was a problem hiding this comment.
Also could we just add a bit to the message here telling consumers what they might have to do because of this change/how it might impact them?
There was a problem hiding this comment.
Do you know what the message should say? Even after checking your React PR patternfly/patternfly-react#10101 and the original issue, I have no clue
There was a problem hiding this comment.
I would just let them know that they will need to update any selectors in their code or tests that depends on that classname being there, along with any snapshot tests that include the class.
|
@wise-king-sullyman I started addressing your comments and then I realized I made a mistake in the component name. The breaking change was on a TreeViewListItem is not directly exported from Do we still need a codemod just in case someone can import it and use it? |
|
It's still pretty much on TreeView, just indirectly. I think a codemod would still be good though since the list item class changes will still potentially impact TreeView users. I would just worry about it in that case though and not worry about someone importing it directly like that. |
|
Oh, I see, you are right. I was way too stuck in the "fixer" mindset from all the other rules 😃 where we only do changes if the exact component is imported. |
adamviktora
force-pushed
the
treeView-warning
branch
from
9626697 to
1253cf0
Compare
3 participants
Closes #601