Eui: EuiContextMenuPanel with EuiLink dom validation error

[walterra]

The docs for EuiBasicTable show two ways of adding actions in the "Adding actions table" section: One with config objects and one with passing a render function that includes EuiLink to create the button. The example with config objects has 5 actions which will get rendered in the popover menu. The example with EuiLink has 2 actions which will get rendered inline in the table.

Now the issue we're facing is: If there are 3 custom actions with render functions based on the EuiLink approach, as expected, they'd get rendered in the popover menu too. But then we'll get a React/DOM error because the popover menu renders nested buttons: One as part the popover menu (without a click event) and a nested one because EuiLink used with onClick gets rendered as a button. (This is similar to the problem described in #1428, I guess the difference there was without more complex logic to render the buttons they could move to use the config object approach)

Here are some more thoughts/suggestions what I can think of how we could solve this:

• The config objects could allow both render and onClick (to do a custom inner rendering without a button and pass the click event to the config object)
• An alternative could be to allow the name attribute of the config object to be something like name: string | ReactElement | (item: T, enabled: boolean) => ReactElement; instead of just name: string;

[chandlerprall]

This is reproducible in the Adding actions to table docs example by adding a 3rd custom action, forcing a popup/context menu when displaying the actions.

Also, in this configuration the control's action fires twice, once when clicking the action and then _collapsed_item_actions_ triggers it again.

_collapsed_item_actions_ is already touching the action component's props, perhaps it is not terrible to expand that logic somewhat to help handle this situation.

[thompsongl]

A couple thoughts:

• Using render should be a option where the consumer can do anything they want but is more or less on their own.
  
  
  
  • Adding onClick to the config item is still too restrictive and the config would need to grow further to include enabled, etc.
  
  
  • We'd need to modify EuiContextMenuItem to not render a button or a (the only two outputs right now) and instead render a div or span so all internal DOM is fair game.
  
  
• Expanding the name item as suggested I think would be enough to solve the majority of use cases. Also passing through the description item as a tooltip content could be considered as an even further advancement towards consumers just using the default config options.

[chandlerprall]

Expanding name sounds good to me. It does not fix the 3+ custom actions bugs, but clears the way for most use cases, as stated above. Only unintended consequence I can see is _default_item_action_ uses the name string as an aria-label and would need to implement useInnerText to extract that value from a component.

[thompsongl]

It does not fix the 3+ custom actions bugs

Right, I think we also want to "modify EuiContextMenuItem to not render a button or a" when render() is in use.

Good point about useInnerText.

[chandlerprall]

@thompsongl @walterra [email protected] is out, with the ability to pass a react node to an action'sname.

[walterra]

Thanks for extending the support for name. I just was looking into picking up the changes in the ML plugin, unfortunately we missed something in the discussion to consider:

name now allows custom JSX, but it doesn't support the callback style item => <span>{item.whatever}</span> to render dynamic JSX based on the item state. We'd need that to render custom tooltip texts depending on the item state (e.g. wether to display if a button is disabled because of permissions or another reason).

Is that callback style you'd consider something worth picking up in a future release?

(It doesn't have to be the callback style, but maybe item could be passed on as a prop for custom JSX to pick up like: ({ item }) = <span>{item.whatever}</span>)

[thompsongl]

Extending further to allow for the callback makes sense to me.

@chandlerprall, you're assigned here, but I'll take on this part.

[chandlerprall]

I'm not sure accepting a component here helps, compared with accepting an element. How would the component be re-triggered? Sounds like we either have an update lifecycle problem where new JSX isn't being rendered, and/or the implementation needs to hook into data updates somehow (e.g. React context). This should be possible:

name: <MyCustomComponentThatSubscribesToStateUpdates/>,

[walterra]

We could subscribe to the overall state using a context, but I'm unsure how we'd solve that for that component to know which _row_ item the instance is referring to on its inner state if that's not passed on?

For example, a user might not be allowed to click a start action and we provide a tooltip with the reason (which could be based on permissions or because a listed job cannot be started again because it finished), so that reason and therefor the rendering of the button could be different for each row, that's why we need the item to make decisions within the component.

[chandlerprall]

Ahh, _that_ callback structure! That the table is not informing you which row name is being rendered for. I understand now, and that request makes sense. Sorry!

[thompsongl]

Expanded functionality was part of the latest EUI release (27.1.0). Should be available in Kibana master (hopefully) in the next week.

[miukimiu]

This issue was fixed by #3739.