elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
54 stars 841 forks source link

[EuiContextMenu] Popover has to close before changes made to items are reflected #2579

Closed lou00011 closed 1 year ago

lou00011 commented 4 years ago

I wanted to make a context menu where each individual menu element can be toggled. I am doing this by changing the icon from empty to check.

        <EuiContextMenuItem
            key='1'
            icon={state?  'check' : 'empty'}
            onClick={e =>  setState(!state))
        >Entry A</EuiContextMenuItem>

However, I found that the context menu doesn't reflect the changes on the fly, but the popover must close and reopen for the change to be reflected.

Is intentional? Is there anyway I can make this element reactive like everything else?

lou00011 commented 4 years ago

Digging deeper, it seems ContextMenuPanel is whats preventing item passed into it being updated unless the popover reopens. This is related to #710, which is fixed (?)

chandlerprall commented 4 years ago

710 is related, but it doesn't fix this instance.

cchaos commented 4 years ago

@lou00011 It seems that if you're just doing selection, you could use EuiSelectable inside of an EuiPopover. There's an example of this concept in the Sizing and containers section of http://localhost:8030/#/forms/selectable

lou00011 commented 4 years ago

@lou00011 It seems that if you're just doing selection, you could use EuiSelectable inside of an EuiPopover. There's an example of this concept in the Sizing and containers section of http://localhost:8030/#/forms/selectable

I have solved the problem by taking all items out of ContextMenuPanel and put them directly into the popover. The overall look isn't affected.

shrey commented 4 years ago

@cchaos Is this issue relevant still? if yes, what all changes do you expect?

chandlerprall commented 4 years ago

Here's a code sandbox demonstrating the issue - https://codesandbox.io/s/competent-williams-z29jd?file=/index.js

When the popup is open, 4 items are shown. The first two use EuiContextMenuItem directly, and tie their check icon to a state variable. The second two use EuiContextMenuItem wrapped in another component, and their icons are tied to a localized state variable. Clicking on either of the last two items toggles that individual item's item as expected. However, clicking on either of the top two items has no apparent effect, until the popover is closed & re-opened, at which point the new state takes effect.

A couple notes:

Definitely acts like something is cloned, or put in a state; however, I don't see either of those concepts in use at a quick glance. This is an odd bug for sure, feel free to ask for help digging through it if you get stuck!

spalger commented 4 years ago

+1 attempting to have a context menu item name change when clicked. I'm using <EuiContextMenu> with the panels prop, changing the items in panel 0, verified that the render is occuring, but the items don't reflect the changes until I close and reopen the popover.

cchaos commented 4 years ago

I'd like to try to better understand the use case for not automatically closing the popover upon selection in the context menu.

The most common use-case, is using it as a menu system. Where the options trigger an action elsewhere, similar to a right click menu or an application menu.

Screen Recording 2020-11-12 at 09 54 30 AM

Another use-case is to make a single-selection type of menu, like pagination. Where the result of the selection is displayed elsewhere but it also only needs to indicate the current selection the first time it pops up.

Screen Recording 2020-11-12 at 09 58 10 AM

--

If the use-cases that are needed here is to make a multi-select dropdown list, we highly suggest the use of EuiSelectable within a regular EuiPopover. The mechanics are such that it does a much better job at relaying selection including accessibility.

If this is not the use-case, please explain further. It'll be much easier for us to understand and help solve.

spalger commented 4 years ago

That's ultimately what I ended up implementing, toggle the state and then close the popover, and it's a better user experience for this use case. I still think it's a bug that the panels/items don't propagate properly when the <ContextMenu> is rerendered.

Only examples I can think of where this is would really be necessary are

2020-11-12 14 38 20

p-young commented 2 years ago

I'm also running into this issue.

My use case is having a theme selector toggle in the context menu. When I toggle it, the accompanying icon and label do not change (but when closed and re-open, show the change).

Is there any plan to fix this or is it just suggested to not use context menu and re-make it using EuiPopover and other components?

cee-chen commented 1 year ago

This appears to have been resolved at some point (React 18 changes? some other set of bug fixes?) - I can't reproduce Chandler's above CodeSandbox code in latest EUI main.

If anyone in this thread is still encountering this issue on latest EUI, please open a new issue with a minimum reproducible sandbox.