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.

Sign up for GitHub

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

Jump to bottom

TreeView: Replace button wrapper around actions with a div #4967

Merged
merged 2 commits into from
Oct 19, 2020
Merged

TreeView: Replace button wrapper around actions with a div #4967

merged 2 commits into from
Oct 19, 2020

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Oct 6, 2020 •
edited
Loading

Resolves #4799.
Relevant core change: patternfly/patternfly#3522

Allows any arbitrary JSX to be used in the action prop on a TreeViewListItem, wrapping it in a plain div with .pf-c-tree-view__action instead of a button. Replaces the existing examples which pass bare icons to this prop with buttons containing those icons.

This makes it possible to render a <Dropdown> in the action container of a tree view item without resulting in a button-in-a-button.

BREAKING CHANGES: existing consumers who want to keep the previous behavior must wrap the contents of their action in a <Button> and pass the values of their actionProps directly to the button.

@mturley mturley mentioned this pull request Oct 6, 2020
Open
@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 6, 2020 •
edited
Loading

PF4 preview: https://patternfly-react-pr-4967.surge.sh
A11y report: https://patternfly-react-pr-4967-a11y.surge.sh

@mcoker
Copy link
Contributor

mcoker commented Oct 6, 2020

@christiemolloy can you verify the structure for the action element? Looks like the action element is in __node, which requires __node be a div (or something non-interactive), and also means the taborder focus goes on the whole node (including the action) first, then the action inside.

Looking at select/app-launcher favorites (https://www.patternfly.org/v4/components/application-launcher#with-favorites-and-search), the main item and associated action are siblings. So focus goes from one to the other without one being nested in the other. It seems like the tree view component is setup to do that - then we could get rid of the __action's negative margins and __node could continue to be a button (or interactive element) as long as it doesn't include a checkbox. Is there any reason we couldn't do that?

redallen
redallen previously approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@mturley
Copy link
Collaborator Author

mturley commented Oct 8, 2020 •
edited
Loading

@mcoker I resolved comments re: variant and aria-label and rebased.

@mturley
Copy link
Collaborator Author

mturley commented Oct 8, 2020

@redallen can you re-approve?

@mturley mturley force-pushed the 4799-tree-view-action-container branch from 40b45fd to 0013995 Compare October 8, 2020 15:59
dlabrecq
dlabrecq requested changes Oct 13, 2020
View reviewed changes
packages/react-core/src/components/TreeView/TreeView.tsx
action?: React.ReactNode;
/** Additional properties of the tree view item action button */
actionProps?: any;
Copy link
Member

Choose a reason for hiding this comment

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

I know TreeView is relatively new, but isn't this a breaking change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it is. It's a beta component though, I thought breaking changes were ok / covered by the disclaimer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@redallen should I make a note/label somewhere indicating the breaking changes, or will that screw up semantic release versioning since it's beta?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make note of it in the OP of this PR for the release notes.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, didn't realize this was still beta

@christiemolloy
Copy link
Member

christiemolloy commented Oct 13, 2020 •
edited
Loading

@christiemolloy can you verify the structure for the action element? Looks like the action element is in __node, which requires __node be a div (or something non-interactive), and also means the taborder focus goes on the whole node (including the action) first, then the action inside.

Looking at select/app-launcher favorites (https://www.patternfly.org/v4/components/application-launcher#with-favorites-and-search), the main item and associated action are siblings. So focus goes from one to the other without one being nested in the other. It seems like the tree view component is setup to do that - then we could get rid of the __action's negative margins and __node could continue to be a button (or interactive element) as long as it doesn't include a checkbox. Is there any reason we couldn't do that?

@mcoker Yeah that makes sense. Originally it was inside because the design specified that you would hover over the node and the action item would appear.

@mcoker
Copy link
Contributor

mcoker commented Oct 13, 2020

@christiemolloy great, thanks! I'll submit a core PR to fix the layout in this PR. Issue - patternfly/patternfly#3592

Screen Shot 2020-10-13 at 10 21 46 AM

@mcoker
Copy link
Contributor

mcoker commented Oct 14, 2020

@mturley patternfly/patternfly#3593 in core has been merged and is in 4.51.2, which should fix the action alignment in this PR.

@mturley
Copy link
Collaborator Author

mturley commented Oct 14, 2020

Great, thanks @mcoker !

@redallen should I bump the pf core version here? not sure if there's a process for that, do we normally do version bumps like that as standalone PRs?

@redallen
Copy link
Contributor

@mturley Usually we let the bot do it (#4983) but you're welcome to do it in this PR as well. Bump it in one package and then run node scripts/verifyPatternflyVersions --fix. I think it's semver comparison package might have broken recently, fixes appreciated!

tlabaj
tlabaj approved these changes Oct 19, 2020
View reviewed changes
Copy link
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

@tlabaj tlabaj merged commit bb84691 into patternfly:master Oct 19, 2020
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@4.8.72
  • @patternfly/react-core@4.66.1
  • @patternfly/react-datetime@4.2.16
  • @patternfly/react-docs@5.10.30
  • @patternfly/react-inline-edit-extension@4.5.129
  • demo-app-ts@4.52.1
  • @patternfly/react-integration@4.52.1
  • @patternfly/react-table@4.18.26
  • @patternfly/react-topology@4.6.37
  • @patternfly/react-virtualized-extension@4.5.117

Thanks for your contribution! 🎉

@mturley mturley deleted the 4799-tree-view-action-container branch October 19, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcoker mcoker mcoker left review comments

@redallen redallen redallen approved these changes

@dlabrecq dlabrecq dlabrecq approved these changes

@tlabaj tlabaj tlabaj approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

@christiemolloy christiemolloy Awaiting requested review from christiemolloy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tree view: add support for action to not be wrapped in a <button>
8 participants
@mturley @patternfly-build @mcoker @christiemolloy @redallen @dlabrecq @tlabaj @kmcfaul