[EuiListGroupItem] Allow onClick with href

[flash1293]

It isn't possible to provide both href and onClick to an EuiListGroupItem element - the onClick prop will be ignored.

[cchaos]

@flash1293 Can you explain a situation when you'd need both. It seems to me that they'd conflict with each other. Is it a link or a button?

href and onClick don't exist as attributes of a single HTML element type. We're just allowing one or the other and we'll use an anchor tag or a button tag appropriately depending on whether the href or onClick is present.

[cchaos]

Edited above href and onClick do exist -> href and onClick don't exist

[flash1293]

@cchaos It is necessary for the "Open Search" panel in discover - it's a link but the flyout closes on click.

[cchaos]

So you're saying that you're providing an href as the target of the click, but that you also need to close the flyout when the link is clicked, which you want to be able to do via onClick?

Sounds to me that you need to just provide an onClick that passes the href location and then in theonClick handler, you close the flyout, then navigate to the href.

[snide]

Agree with @cchaos here. href and onClick don't really mix and shouldn't be used in tandem because you can't guarantee they'll execute in any known order. More complicated actions should just use onClick and perform actions within a handler.

[flash1293]

Sounds to me that you need to just provide an onClick that passes the href location and then in theonClick handler, you close the flyout, then navigate to the href.

But if I do this, I can't do "open in new tab" from this item.

[cchaos]

You should be able to with window.open('https://...', '_blank');

[flash1293]

You should be able to with window.open('https://...', '_blank');

What I meant is that the user is able to decide whether they want to open the link in a new tab or the same tab. Either way I want to trigger some side effect (in this case closing a flyout) as a response to the navigating - in this case the navigating doesn't close the flyout even when staying in the same tab.

I think it boils down to a pro and a counter argument:

• pro: It's like the native elements behave - I can have both a href and an onClick on a native anchor element, so I (as an EUI user) expect the respective EUI elements to behave like this - at least it should be documented https://elastic.github.io/eui/#/navigation/button.
• contra: href and onClick is a code smell in the first place and thus shouldn't be allowed. (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a)

I would lean towards the pro argument since while it might not always be the cleanest way to use href and onClick together, there might be some exotic use cases which need it, but I don't hold a strong opinion about this.

[snide]

We ran through this as a group today in our design meeting. We'd like to keep onClick and href distinct and follow closer to standard specs. We do this a lot in multiple components (both EuiLink and EuiButton off hand) so we'd have to address this in lots of places. The reasoning is this suggests a proper pattern that leads to consistent HTML accessibility (buttons and anchors are different) across the UI. We're a little worried about people misusing this kind of ability and mixing up their concerns. We really haven't run into this use case before, and I don't think it's going to be too common so I think it's OK to just build your own pattern outside of EUI for this.

Suggestion for now would be that if you need this kind of functionality to just use a secondary icon next to the link to suggest opening it in another tab and using a separate action. popout works for this, but we can probably make a better "link" style icon. It's just a very narrow use case. Even a second "Go here (or open in a new tab)" link next to it works if you really think this is going to be common.

[stacey-gammon]

fwiw I am hitting this need as well. I think it may be become more common because you should use onClick to navigate to internal apps without causing a full page refresh, but we also want to support "open in new tab".

Drilldown code needs this as well. Both onClick and href are set on EuiContextMenuPanelItemDescriptor, it's not hitting the ts error though for some reason. https://github.com/elastic/kibana/blob/master/src/plugins/ui_actions/public/context_menu/build_eui_context_menu_panels.tsx#L118

[stacey-gammon]

I'm getting around it by using your pop out icon, but it feels a bit weird because most users would use right click -> open in new tab.

[cchaos]

Let's go ahead an implement allowing both onClick and href on EuiListGroupItems knowing that we have the lint message they can ignore, but at least we don't block usage or force consumers to "hack" it.

[chandlerprall]

knowing that we have the lint message they can ignore

We'll need to update the eslint plugin to include EuiListGroupItems, and then make sure to publish that package and update it in Kibana.

eui/packages/eslint-plugin/rules/href_or_on_click.js

Line 1 in 336666a

const componentNames = ['EuiButton', 'EuiButtonEmpty', 'EuiLink'];

[github-actions]

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

[flash1293]

Bumping this - it’s still valid. There’s a workaround on Kibana side, but it would be nice to have it supported natively

[github-actions]

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

[snide]

Added a skip so the bot doesn't engage.

[cee-chen]

This appears to be have been resolved in #1933. I can't reproduce it in latest prod at least - passing both href='/' onClick={(e) => e.preventDefault() works as expected.

Edit: I also don't see a type error for passing both href and onClick, so I think that has been resolved as well.