feat(menus): custom menus updates #9014

kmcfaul · 2023-05-01T14:24:02Z

What: Closes #8899

Renames demos to not use the word 'composable'
Fixes drilldown demo. The if block controlling height updates was no longer being reached because the initial heights being returned for non-visible menus was updated to be non-zero (because of padding). The issue mentions flyout also needed to be fixes but it looks like its working. Maybe it was fixed as byproduct of other updates, lmk if anything seems broken with it.
Removes redundant demos (basic select and dropdown demos that are already covered in their respective docs)
Converts the remaining demos to not use Popper. Demos now use Select or Dropdown, except for the flyout, drilldown, and tree view demos. Flyout and drilldown make extensive use of extra menu props not explicitly exposed by Select or Dropdown (though it would still work), as well as needing props on MenuContent which isn't accessible by Select/Dropdown currently (for drilldown). The tree view demo doesn't use menu at all. To keep from using Popper directly still, I added a MenuContainer component to menu to act as a similar wrapper for Popper like Select/Dropdown. The application launcher demo and context selector demo end up being slightly different in the DOM than previously because MenuContent is internal to Select/Dropdown and both had elements (MenuSearch, MenuFooter) that previously were outside of the content. I also had to add an onOpenChangeKeys prop to Dropdown because these two require the Tab key to access different sections of the menu (search and footer) instead of closing the menu. Lmk if it would be better to use MenuContainer again and stick with the menu components.
Adds demo pages to context selector, application launcher, and options menu docs.
Adds favorites demo

patternfly-build · 2023-05-01T14:32:40Z

Preview: https://patternfly-react-pr-9014.surge.sh

A11y report: https://patternfly-react-pr-9014-a11y.surge.sh

mcarrano · 2023-05-01T20:53:16Z

The preview is broken (no left nav). Needs to be rebased?

nicolethoen

I think the new demos - like the Options Menu demo and App launcher demo - need a sentence at the top to explain that, since the old version is deprecated, the component can still be built using the new Dropdown/Select (as demonstrated in the demo)

thatblindgeye

More of a nit, but if we want to move away from using "composable" in example names, we could update the directory from demos/ComposableMenu to demos/CustomMenu.

packages/react-core/src/components/Dropdown/Dropdown.tsx

packages/react-core/src/components/Menu/MenuContainer.tsx

packages/react-core/src/demos/ComposableMenu/OptionsMenu.md

packages/react-core/src/demos/ComposableMenu/examples/DateSelectDemo.tsx

packages/react-core/src/demos/ComposableMenu/examples/DrilldownMenuDemo.tsx

packages/react-core/src/demos/ComposableMenu/examples/FavoritesDemo.tsx

packages/react-core/src/demos/ComposableMenu/examples/ActionsMenuDemo.tsx

packages/react-core/src/demos/ComposableMenu/examples/DateSelectDemo.tsx

mcarrano

This looks great. I have nothing to add beyond the issues already raised by @thatblindgeye .

packages/react-core/src/components/Menu/MenuContainer.tsx

nicolethoen

Can we add Props tables for components used to build these demos to this page? In particular i'm interested in seeing MenuContainer documented, but since there are not too many components used in these demos, a few other props tables may be helpful, too?

kmcfaul · 2023-05-11T17:21:38Z

@nicolethoen Do you also want to add prop tables to the custom menus page? I added them to the specific demos for the deprecated components.

nicolethoen · 2023-05-11T19:38:43Z

@kmcfaul i'd vote for the MenuContainer at least to be documented.
Do any of our Menu examples include a MenuContainer? I'd only include that props table on the Menu docs if they use it in the examples.

packages/react-core/src/demos/CustomMenus/ContextSelector.md

packages/react-core/src/demos/CustomMenus/CustomMenus.md

packages/react-core/src/demos/CustomMenus/ApplicationLauncher.md

packages/react-core/src/demos/CustomMenus/CustomMenus.md

packages/react-core/src/demos/CustomMenus/OptionsMenu.md

kmcfaul · 2023-05-15T14:13:01Z

@nicolethoen No just the custom menus page uses the new container (and I've included the props table there).

mcarrano

This looks good to me. Only have one question. I see that we've added React Demo tabs for the Context selector and Application launcher components (as intended), but still also have those demos on the Custom menus page. Did we intend to duplicate these on the Custom menu page? Can't remember our earlier convo about this. What are your thoughts @nicolethoen ?

packages/react-core/src/demos/CustomMenus/CustomMenus.md

packages/react-core/src/demos/CustomMenus/ApplicationLauncher.md

packages/react-core/src/demos/CustomMenus/ContextSelector.md

packages/react-core/src/demos/CustomMenus/ApplicationLauncher.md

packages/react-core/src/demos/CustomMenus/ContextSelector.md

packages/react-core/src/demos/CustomMenus/OptionsMenu.md

kmcfaul · 2023-05-17T16:41:44Z

@mcarrano @nicolethoen I believe we landed on having the app launcher/context selector/options menu in both places. The code is only in one location it's just referenced in two places. I can remove them from the custom menus if we want though.

mcarrano · 2023-05-17T17:39:16Z

I believe we landed on having the app launcher/context selector/options menu in both places. The code is only in one location it's just referenced in two places. I can remove them from the custom menus if we want though.

I'm OK to leave it, but just wanted to raise the question.

tlabaj

If we are going to have this menu container, shouldn't our Select and dropdown use it?

packages/react-core/src/components/Dropdown/Dropdown.tsx

packages/react-core/src/components/Menu/examples/Menu.md

kmcfaul · 2023-05-17T21:24:03Z

If we are going to have this menu container, shouldn't our Select and dropdown use it?

We can refactor them, but as it's internal maybe this could be a follow up?

The benefit of having separate implementations is if we want to differentiate how a Select vs a Dropdown vs a generic Menu behaves by default on the other hand.

tlabaj

looks good, I just wonder if the Menu container should be Beta. @mcarrano thoughts?

nicolethoen · 2023-05-18T01:45:12Z

looks good, I just wonder if the Menu container should be Beta. @mcarrano thoughts?

Generally, since we will be doing more frequent major releases, I don't feel the need to mark everything beta - unless it's a completely optional enhancement or experimental. I've been made aware that Extensions cannot use beta components at all, because they cannot introduce breaking changes - and have come to realize that products are generally very hesitant to adopt any beta solutions. If we want people to use this MenuContent as they build out custom Select implementations, because it is our recommended solution, i'd advocate for promoting it.

I understand any hesitation though.

tlabaj · 2023-05-18T13:03:20Z

Generally, since we will be doing more frequent major releases, I don't feel the need to mark everything beta - unless it's a completely optional enhancement or experimental. I've been made aware that Extensions cannot use beta components at all, because they cannot introduce breaking changes - and have come to realize that products are generally very hesitant to adopt any beta solutions. If we want people to use this MenuContent as they build out custom Select implementations, because it is our recommended solution, i'd advocate for promoting it.

I understand any hesitation though.

I think this is a discussion we should have as a team. I will add it to the agenda.

nicolethoen · 2023-05-18T13:13:46Z

We can also add/remove a beta tag later and not hold this PR up.

kmcfaul · 2023-05-18T14:17:03Z

Rebased with the select PR merge, and re added a beta tag to the drilldown example that I think I removed in error during a previous rebase (just double checked the promotion PR and its still in beta from that).

tlabaj · 2023-05-18T17:27:09Z

packages/react-core/src/components/Menu/MenuContainer.tsx

+  /** Callback to change the open state of the menu.
+   * Triggered by clicking outside of the menu, or by pressing any keys specificed in onOpenChangeKeys. */
+  onOpenChange?: (isOpen: boolean) => void;
+  /** @beta Keys that trigger onOpenChange, defaults to tab and escape. It is highly recommended to include Escape in the array, while Tab may be omitted if the menu contains non-menu items that are focusable. */


Can we mark this whole component Beta for now and remove this tag on the prop.

patternfly-build · 2023-05-18T19:07:19Z

Your changes have been released in:

@patternfly/react-code-editor@5.0.0-alpha.111
@patternfly/react-core@5.0.0-alpha.110
@patternfly/react-docs@6.0.0-alpha.118
demo-app-ts@5.0.0-alpha.94
@patternfly/react-table@5.0.0-alpha.112

Thanks for your contribution! 🎉

* feat(menus): custom menus updates * fix merge update * rebase and deprecated blurb * rename directory, remove dupe focusing, update desc * add beta to keys * update prop desc * add prop tables, fix focusing bug * content updates * content updates * move prop, add popper props * update wording * add beta note to prop table

kmcfaul force-pushed the custom-menu-cleanup branch from e062ef6 to db4b98e Compare May 1, 2023 14:32

tlabaj requested review from edonehoo, mcarrano and mmenestr May 1, 2023 17:49

tlabaj added the content review label May 1, 2023

tlabaj requested review from thatblindgeye and tlabaj May 1, 2023 17:51

nicolethoen reviewed May 3, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from 1aaf12e to 4382dd0 Compare May 3, 2023 18:21

thatblindgeye requested changes May 3, 2023

View reviewed changes

mcarrano reviewed May 3, 2023

View reviewed changes

thatblindgeye mentioned this pull request May 3, 2023

feat(Select): add examples, add popper interface, fix focus #8992

Merged

kmcfaul force-pushed the custom-menu-cleanup branch from e6059d0 to 7796c05 Compare May 8, 2023 15:05

thatblindgeye approved these changes May 9, 2023

View reviewed changes

packages/react-core/src/components/Menu/MenuContainer.tsx Show resolved Hide resolved

kmcfaul force-pushed the custom-menu-cleanup branch from ce42c5e to 2d1482f Compare May 9, 2023 15:02

nicolethoen reviewed May 9, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from 2820de1 to 63718fb Compare May 11, 2023 17:23

edonehoo reviewed May 15, 2023

View reviewed changes

nicolethoen approved these changes May 15, 2023

View reviewed changes

mcarrano reviewed May 17, 2023

View reviewed changes

edonehoo reviewed May 17, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from 80a4cab to 9df2771 Compare May 17, 2023 16:39

kmcfaul force-pushed the custom-menu-cleanup branch from 9df2771 to 7705150 Compare May 17, 2023 17:24

edonehoo approved these changes May 17, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from 7705150 to c1135c4 Compare May 17, 2023 20:40

tlabaj requested changes May 17, 2023

View reviewed changes

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved

packages/react-core/src/components/Menu/examples/Menu.md Show resolved Hide resolved

kmcfaul force-pushed the custom-menu-cleanup branch from aaec02c to ab19037 Compare May 17, 2023 21:29

tlabaj reviewed May 18, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from ab19037 to d992f81 Compare May 18, 2023 14:15

tlabaj reviewed May 18, 2023

View reviewed changes

kmcfaul added 12 commits May 18, 2023 13:55

feat(menus): custom menus updates

bb333f3

fix merge update

ed0a7c3

rebase and deprecated blurb

635081a

rename directory, remove dupe focusing, update desc

f467daa

add beta to keys

a0dcf57

update prop desc

517628b

add prop tables, fix focusing bug

fbf62d2

content updates

99ec946

content updates

d041ff1

move prop, add popper props

2d90d99

update wording

c8c2ea9

add beta note to prop table

42cf3a4

tlabaj approved these changes May 18, 2023

View reviewed changes

kmcfaul force-pushed the custom-menu-cleanup branch from d992f81 to 42cf3a4 Compare May 18, 2023 18:07

mcarrano approved these changes May 18, 2023

View reviewed changes

tlabaj merged commit 4055504 into patternfly:v5 May 18, 2023

feat(menus): custom menus updates #9014

feat(menus): custom menus updates #9014

Uh oh!

Conversation

kmcfaul commented May 1, 2023

Uh oh!

patternfly-build commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcarrano commented May 1, 2023

Uh oh!

nicolethoen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

kmcfaul commented May 11, 2023

Uh oh!

nicolethoen commented May 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmcfaul commented May 15, 2023

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmcfaul commented May 17, 2023

Uh oh!

mcarrano commented May 17, 2023

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kmcfaul commented May 17, 2023

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented May 18, 2023

Uh oh!

tlabaj commented May 18, 2023

Uh oh!

nicolethoen commented May 18, 2023

Uh oh!

patternfly-build commented May 1, 2023 •

edited

Loading

nicolethoen left a comment •

edited

Loading