Skip to content

add submenu support to resource action menus #3922

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
Merged
openshift-merge-robot merged 3 commits into from
Jan 14, 2020
Merged

add submenu support to resource action menus #3922

openshift-merge-robot merged 3 commits into from
Jan 14, 2020

Conversation

@christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Jan 11, 2020

Fixes https://issues.redhat.com/browse/ODC-2712
Related to work in topology view to have a way to access the Add actions in-context: #3918

Problem

Kebab actions only support a flat list.
Patternfly does not have any sub menu support either.
Many more actions are being added to resources and a flat list of actions will become much too large.

Solution

This PR builds off the earlier support for popper.js based menus. These menus automatically align to the left or right depending on where space is available. The preference is set to always open to the right if there is available space. This is why in the screenshots for the kebab and actions dropdown, the 2nd nested menu appears out to the right. The nesting of sub menus multiple levels deep is shown in the screenshots because the solution has no restriction on depth.

There is some duplicate code between kebab and topology in the popper setup but they are are based on two very different menu implementations. Also, we are trying to keep topology pure from console dependencies to eventually push it to PF.

The way to specify actions to appear in a sub menu is by using the new path prop of the KebabOption. The path is a / separated string where each segment denotes a new sub menu entry. Eg. Menu 1/Menu 2/Menu 3

There is keyboard support as well.

  • Right-Arrow | Enter opens a sub menu
  • Left-Arrow closes a sub menu
  • Esc continues to close the entire menu for kebab and topology while only closing sub menus in the dropdown. This is because of the way dropdowns have a very different implementation.

image

image

image

Keyboard navigation:
submenus

cc @spadgett @karthikjeeyar @andrewballantyne

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/core Related to console core functionality component/dev-console Related to dev-console component/shared Related to console-shared labels Jan 11, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2020
abhinandan13jan
abhinandan13jan reviewed Jan 13, 2020
View reviewed changes
frontend/packages/topology/src/components/contextmenu/ContextSubMenuItem.tsx Outdated
Copy link
Contributor

@abhinandan13jan abhinandan13jan Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

should there be a keycode 37 check for closing the submenu i.e setOpen(false)?

Copy link
Contributor Author

@christianvogt christianvogt Jan 13, 2020

Choose a reason for hiding this comment

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

Left arrow is being captured in the scope of the sub menu itself. See code below on line 80 of this file.

@karthikjeeyar karthikjeeyar mentioned this pull request Jan 13, 2020
Merged
@karthikjeeyar
Copy link
Contributor

karthikjeeyar commented Jan 13, 2020

/retest

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 13, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 13, 2020
@karthikjeeyar
Copy link
Contributor

karthikjeeyar commented Jan 13, 2020

Verified locally
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 13, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, karthikjeeyar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 13, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b0be943 into openshift:master Jan 14, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 14, 2020
@christianvogt christianvogt deleted the submenus branch August 20, 2020 16:31
@christianvogt christianvogt restored the submenus branch August 20, 2020 16:31
@christianvogt christianvogt deleted the submenus branch August 20, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@invincibleJai invincibleJai Awaiting requested review from invincibleJai

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

1 more reviewer

@abhinandan13jan abhinandan13jan abhinandan13jan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@karthikjeeyar karthikjeeyar

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v4.4

Development

Successfully merging this pull request may close these issues.

8 participants

@christianvogt @karthikjeeyar @andrewballantyne @openshift-ci-robot @openshift-bot @abhinandan13jan @spadgett @openshift-merge-robot