-
Notifications
You must be signed in to change notification settings - Fork 356
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
chore(TreeView): converted examples to TypeScript #8448
Conversation
Preview: https://patternfly-react-pr-8448.surge.sh A11y report: https://patternfly-react-pr-8448-a11y.surge.sh |
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.
Good work on this so far! A few mostly small things:
packages/react-core/src/components/TreeView/examples/TreeViewDefault.tsx
Outdated
Show resolved
Hide resolved
// Ignore folders for selection | ||
if (treeViewItem && !treeViewItem.children) { | ||
setActiveItems([treeViewItem]); | ||
} |
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 this change addressing another issue?
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'm not sure what you're asking
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 wasn't sure where this change came from/how it relates to the example conversion.
If it wasn't for/from some other issue can you explain why it was required?
packages/react-core/src/components/TreeView/examples/TreeViewSelectableNodes.tsx
Outdated
Show resolved
Hide resolved
console.log(checked); | ||
|
||
const checkedItemTree = this.options | ||
.map(opt => Object.assign({}, opt)) | ||
.filter(item => this.filterItems(item, treeViewItem)); | ||
const flatCheckedItems = this.flattenTree(checkedItemTree); | ||
console.log('flat', flatCheckedItems); | ||
|
||
this.setState( | ||
prevState => ({ | ||
checkedItems: checked | ||
? prevState.checkedItems.concat( | ||
flatCheckedItems.filter(item => !prevState.checkedItems.some(i => i.id === item.id)) | ||
) | ||
: prevState.checkedItems.filter(item => !flatCheckedItems.some(i => i.id === item.id)) | ||
}), | ||
() => { | ||
console.log('Checked items: ', this.state.checkedItems); |
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'm curious why you removed these console logs.
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.
It doesn't let me commit with console logs
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.
Even if you add an eslint-disable-next-line no-console
linter disabling comment before it?
const checkedItemTree = options.map(opt => Object.assign({}, opt)).filter(item => filterItems(item, treeViewItem)); | ||
const flatCheckedItems = flattenTree(checkedItemTree); | ||
|
||
setCheckedItems( |
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 should use an updater function for the current state of checkedItems
here as well.
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'm not entirely sure how to do that since it relies on the "checked" boolean
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.
🤔 are you not able to access that variable from within an updater function?
packages/react-core/src/components/TreeView/examples/TreeViewCheckbox.tsx
Show resolved
Hide resolved
packages/react-core/src/components/TreeView/examples/TreeViewIconActionItems.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/TreeView/examples/TreeViewMemo.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/TreeView/examples/TreeViewMemo.tsx
Outdated
Show resolved
Hide resolved
Changes so far look good, left a few replies. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@kev-kim are you interested in trying to finish up this PR? One note, you would need to rebase this PR and target the v5 branch rather than main. |
Closing in favor of #9286 |
What:
Additional issues: