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

Add support for href, target, and rel properties for EuiContextMenu items. #911

Merged
merged 2 commits into from
Jun 8, 2018

Conversation

cjcenizal
Copy link
Contributor

@jen-huang This will give us the option to specify hrefs for the index pattern creation options, instead of using onClick in conjunction with kbnUrl (if we decide that's better).

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a useful enhancement, regardless of use for index pattern creation!

LGTM

ref={buttonRef}
{...rest}
>
<span className="euiContextMenu__itemLayout">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reduce duplication, we could do:

const buttonInner = (
  <span className="euiContextMenu__itemLayout">
    {iconInstance}
    <span className="euiContextMenuItem__text">
      {children}
    </span>
    {arrow}
  </span>
);

And use {buttonInner} inside both <a> and <button>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good rule of thumb because it not only reduces duplication, makes it easier to read, but also makes it easier to update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks peeps! Good suggestion.

@cjcenizal cjcenizal merged commit 5b614a2 into elastic:master Jun 8, 2018
@cjcenizal cjcenizal deleted the context-menu-item-href branch June 8, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants