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.

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

[TASK](ui): rework Menu to become PopupMenu #529

Open
9 of 14 tasks
Tracked by #62
franzheidl opened this issue Oct 17, 2024 · 2 comments
Open
9 of 14 tasks
Tracked by #62

[TASK](ui): rework Menu to become PopupMenu #529

franzheidl opened this issue Oct 17, 2024 · 2 comments
Assignees
Labels
ui-components All tasks related to juno-ui-components library

Comments

@franzheidl
Copy link
Contributor

franzheidl commented Oct 17, 2024

Task Description
The current Menu (and ContextMenu) component is flawed in several ways, and needs to be reworked as a more general PopupMenu component.

The new PopupMenu component should expose PopupMenu.Toggle, PopupMenu.Menu, PopupMenu.Item and PopupMenu.Section subcomponents.

The new PopupMenu component should accept a toggle and a menu as props. It should be possible to pass these as slotted props, or as children:

<PopupMenu toggle={<MyToggle />} menu={<MyMenu />} />

// or

<PopupMenu>
  <MyToggle />
  <MyMenu />
</PopupMenu>

Users can use subcomponents as exposed by PopupMenu (recommended), or their own custom components as toggle and menu respectively.

<PopupMenu toggle={<PopupMenu.Toggle />} menu={<PopupMenu.Menu><PopupMenu.Item /></PopupMenu.Menu} />

// or

<PopupMenu>
  <PopupMenu.Toggle />
  <PopupMenu.Menu>
    <PopupMenu.Item />
  </PopupMenu.Menu>
</PopupMenu>

Additional Requirements

  • Both toggle and menu should be optional, resulting in the component rendering always.
  • When no toggle is passed, PopupMenu should render an "overflow" / "kebap" icon as a toggle, allowing it to effectively replace the curren tContextMenu component
  • When no menu is passed, the toggle should still render and all necessary events should fire anyways.
  • When passing children to the PopupMenu, the bevaviour should be as follows:
    • When subcomponents are being used, we render according to type / element, regardless of sequence (edge case: more than one PopupMenu and/or PopupMenu.Menu each: render the first one met, ignore the rest)
    • When non-subcomponent / custom component children are used, sequence matters: the first child will be rendered as toggle, the second as menu. Any more / others will be ignored.
    • When toggle and/or menu are being passed as both props and children, props win.
    • Clicking outside an open menu should close the menu

Sub-tasks

  • Create a new component in TS as a generic PopupMenu
  • Design a component architecture that wraps Headless Menu
  • Use sub components
  • Use the new PortalProvider to render the Menu into
  • Figure out and implement working, reliable positioning of the menu in the Portal
  • Make component render as an overflow menu per default, effectively replacing ContextMenu
  • Allow passing any Juno icon instead of default three vertical dots icon
  • Allow passing a custom component as a toggle as a prop into a slot
  • Make sure all onClick handlers on custom component toggle still run
  • Allow passing a PopupMenu.Menu element as a slot, containing the PopupMenu.Item elements
  • Allow passing the toggle as a child, too.
  • Allow passing the menu as a child, too, so effectively emulate the architecture/API of headless UI dropdown menu
  • Deprecate current Menu and ContextMenu components (as an option, we could ffw ContextMenu to PopupMenu internally, and display a deprecation warning, in effect not using the old component at all even in the deprecation phase)
  • Implement closing on click outside, make potentially configurable via a prop (closeOnClickOutside?: true)

Related Issues
DONE: Will require migration of Menu and related MenuItem, MenuSection components to TypeScript #237

@franzheidl franzheidl added the ui-components All tasks related to juno-ui-components library label Oct 17, 2024
@franzheidl franzheidl self-assigned this Oct 17, 2024
@edda edda changed the title feat(ui): rework ContextMenu to become PopupMenu [TASK](ui): rework ContextMenu to become PopupMenu Oct 17, 2024
@franzheidl franzheidl changed the title [TASK](ui): rework ContextMenu to become PopupMenu [TASK](ui): rework Menu to become PopupMenu Nov 6, 2024
@franzheidl
Copy link
Contributor Author

franzheidl commented Nov 20, 2024

The intended flexibility in terms of API and usage as outlined above leads to a large amount of complexity in the component, especially accepting any kind of component as a toggle and potentially menu, as well as allowing to pass them as children or as slotted props. Also, there will be edge cases, some of which may prove impossible to handle nicely.

Current WIP is on https://github.com/cloudoperators/juno/tree/popupmenu-529

(This does not yet cover rendering children properly, and already is a very large, complex component)

I am currently considering cutting some requirements and building a thin wrapper around headless ui menu first, accepting only subcomponents as children.

Having a simpler solution in place would allow us to see how well this works for our users and address shortcomings and requirements for additional functionality and features later.

@edda when you have some time after you're back from holidays would love to discuss and hear what you think.

@franzheidl
Copy link
Contributor Author

A WiP alternative, much simpler implementation of a PopupMenu component as a thin wrapper around headless ui menu can be found here: https://github.com/cloudoperators/juno/tree/popupmenu-simple-529

This does not have any styling yet, but is helpful in understanding why this component is difficult to build:

When just wrapping headless ui menu, there is no way of passing an open prop, as headless ui menu manages its state internally and does not expose such a functionality. If we want to allow for passing an open prop, and there is a strong argument to do that for the sake of consistency in our system, we would need to override headless ui's internal state with our own, and render the menu as static. Th edownside of this approach is we would loose a lotof headless-ui menu's inbuilt functionality, e.g. clsoing the menu on clicking outside or clicking an item would need re-implementing in our component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui-components All tasks related to juno-ui-components library
Projects
None yet
Development

No branches or pull requests

1 participant