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

EuiBadge should allow for href #2831

Closed
cchaos opened this issue Feb 6, 2020 · 18 comments · Fixed by #2954 or #3009
Closed

EuiBadge should allow for href #2831

cchaos opened this issue Feb 6, 2020 · 18 comments · Fixed by #2954 or #3009
Assignees

Comments

@cchaos
Copy link
Contributor

cchaos commented Feb 6, 2020

Currently it only accepts onClick but badges can also link to other pages as well. It should accept href similar to EuiButton.

@cchaos
Copy link
Contributor Author

cchaos commented Feb 19, 2020

Hi @aryamanpuri , yes please do! Essentially the component needs to accept an href prop and then output the right html element.

  • If the consumer passes an href the component should render as link tag <a>
  • If the consumer passes an onClick the component should render as a button tag <button>

You can essentially look at how EuiButton handles the render:

// <a> elements don't respect the `disabled` attribute. So if we're disabled, we'll just pretend
// this is a button and piggyback off its disabled styles.
if (href && !isDisabled) {
const secureRel = getSecureRelForTarget({ href, target, rel });
return (
<a
className={classes}
href={href}
target={target}
rel={secureRel}
ref={buttonRef as Ref<HTMLAnchorElement>}
{...rest as AnchorHTMLAttributes<HTMLAnchorElement>}>
{innerNode}
</a>
);
}
let buttonType: ButtonHTMLAttributes<HTMLButtonElement>['type'];
return (
<button
disabled={isDisabled}
className={classes}
type={type as typeof buttonType}
ref={buttonRef as Ref<HTMLButtonElement>}
{...rest as ButtonHTMLAttributes<HTMLButtonElement>}>
{innerNode}
</button>

And we have two helper services for the TS types you can reuse similar to EuiButton here:

type EuiButtonPropsForAnchor = PropsForAnchor<
EuiButtonProps,
{
buttonRef?: Ref<HTMLAnchorElement>;
}
>;
type EuiButtonPropsForButton = PropsForButton<
EuiButtonProps,
{
buttonRef?: Ref<HTMLButtonElement>;
}
>;
export type Props = ExclusiveUnion<
EuiButtonPropsForAnchor,
EuiButtonPropsForButton
>;

Be sure to check the whole file as not everything was captured in those permalinks.

Thanks!

@shakti97
Copy link

may I work on this?

@shakti97
Copy link

I checked the code, button component is already sufficient to handle render of anchor or button based on the href prop, so just I need to add the optional href in the badge prop and add it to the button .
@cchaos please correct me if I m going wrong.

@cchaos
Copy link
Contributor Author

cchaos commented Feb 21, 2020

The EuiButton component does correclty handle this situation, but we essentially need to translate that same type of code to EuiBadge to handle it as well since EuiBadge does not extend EuiButton.

@RisingGeek
Copy link

Hi @cchaos , I was looking at the EuiButton component and was wondering what does exclusive union does?
export type Props = ExclusiveUnion<EuiButtonPropsForAnchor,EuiButtonPropsForButton>;

@thompsongl
Copy link
Contributor

thompsongl commented Feb 21, 2020

@RisingGeek There's a good, full description of ExclusiveUnion in the code comments. (And another for the specific case you mention)

The idea is that we needed a way for a component with multiple potential element targets to ensure that the attributes for those discrete elements don't get mixed. For instance, a simple union of HTMLAnchorElement | HTMLButtonElement might allow for anhref attribute to be passed to a <button />, which is invalid and something we want to avoid. ExclusiveUnion is our method of requiring that consumers pass props from either HTMLAnchorElement or HTMLButtonElement, but not a combination of the two.

@shakti97
Copy link

The EuiButton component does correclty handle this situation, but we essentially need to translate that same type of code to EuiBadge to handle it as well since EuiBadge does not extend EuiButton.

okay will try to do that

@RisingGeek
Copy link

@RisingGeek There's a good, full description of ExclusiveUnion in the code comments. (And another for the specific case you mention)

The idea is that we needed a way for a component with multiple potential element targets to ensure that the attributes for those discrete elements don't get mixed. For instance, a simple union of HTMLAnchorElement | HTMLButtonElement might allow for anhref attribute to be passed to a <button />, which is invalid and something we want to avoid. ExclusiveUnion is our method of requiring that consumers pass props from either HTMLAnchorElement or HTMLButtonElement, but not a combination of the two.

Got it. Thanks

@Mallik813
Copy link

@cchaos
Is this issue not closed yet? It's been 16 days since it was open? If it is not closed yet, can I take up this issue?
Thanks

@shakti97
Copy link

Hi @Mallik813 , I'm currently working on this. Could you please check some other issue

@Mallik813
Copy link

@shakti97 sure!

@anishagg17
Copy link
Contributor

@shakti97 May I take over this issue?

@anishagg17
Copy link
Contributor

@chandlerprall may i take this issue now?

@chandlerprall
Copy link
Contributor

@anishagg17 Go ahead

@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2020

#2954 Cleaned up EuiButton code nicely, but it didn't address this issue. EuiBadge is a completely separate component that needs the same implementation as EuiButton. It does not re-use EuiButton or logic.

@cchaos cchaos reopened this Mar 2, 2020
@anishagg17
Copy link
Contributor

@cchaos can I go for cleaning badge groups too??

@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2020

EuiBadgeGroup doesn't actually contain any EuiBadge's. Only EuiBadge needs to be addressed by this issue.

@anishagg17
Copy link
Contributor

OKay , I would like to work on it @cchaos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants