Skip to content

fix(Treeview): ignore folder selection in basic example#7906

Merged
tlabaj merged 2 commits intopatternfly:mainfrom
kmcfaul:treeview-selected-state
Sep 12, 2022
Merged

fix(Treeview): ignore folder selection in basic example#7906
tlabaj merged 2 commits intopatternfly:mainfrom
kmcfaul:treeview-selected-state

Conversation

@kmcfaul
Copy link
Copy Markdown
Contributor

@kmcfaul kmcfaul commented Aug 30, 2022

What: Closes #7785

Folder selection looked like it wasn't being persisted because the parent folder was technically getting selected (but this doesn't have visual styling by default).

@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

nice :)

Copy link
Copy Markdown
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @kmcfaul !

Copy link
Copy Markdown
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM, but should we update all of the non-expandable/selectable examples?

@mcoker
Copy link
Copy Markdown
Contributor

mcoker commented Sep 7, 2022

Just curious - what's the use case for calling onSelect on an expandable node that isn't selectable? I ask because it looks like we don't call onSelect when you click on the toggle in a selectable node. Clicking on a non-selectable node is basically just a toggle, too, though I'm sure there are use cases I'm not aware of.

@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Sep 8, 2022

The callback can allow the user to track what folder is opened and execute logic based on that, and I think the initial design of the treeview allowed folders to be selected (and highlighted) so it's also a holdover from that.

Copy link
Copy Markdown
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

@kmcfaul & @mcarrano, @mcoker had a good question - should all examples be updated here? not just the basic example?

@kmcfaul kmcfaul force-pushed the treeview-selected-state branch from b56b2a9 to 0ce08c3 Compare September 9, 2022 15:45
@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Sep 9, 2022

I went ahead and updated the handful of other tree view examples that only use single selection of leaf nodes to disallow selecting the folder.

Copy link
Copy Markdown
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM - though the memoization example still attempts to select expandable nodes, should we update that one, too?

@tlabaj tlabaj merged commit b16b224 into patternfly:main Sep 12, 2022
@patternfly-build
Copy link
Copy Markdown
Collaborator

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.76.6
  • @patternfly/react-catalog-view-extension@4.88.6
  • @patternfly/react-charts@6.90.6
  • @patternfly/react-code-editor@4.78.6
  • @patternfly/react-console@4.88.6
  • @patternfly/react-core@4.237.6
  • @patternfly/react-docs@5.98.6
  • @patternfly/react-icons@4.88.6
  • @patternfly/react-inline-edit-extension@4.82.6
  • demo-app-ts@4.197.6
  • @patternfly/react-integration@4.199.6
  • @patternfly/react-log-viewer@4.82.6
  • @patternfly/react-styles@4.87.6
  • @patternfly/react-table@4.106.6
  • @patternfly/react-tokens@4.89.6
  • @patternfly/react-topology@4.84.6
  • @patternfly/react-virtualized-extension@4.84.6
  • transformer-cjs-imports@4.75.6

Thanks for your contribution! 🎉

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.

Bug - Tree view - selected state doesn't persist if parent node is collapsed

8 participants