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

[EuiBetaBadge] Add color and size props and support for click event #4798

Merged
merged 17 commits into from
May 28, 2021

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented May 14, 2021

Summary

  • Added the color prop and added two more color options (subdued and accent) and turned previous default into hollow.
  • Added the size prop.
  • Allowed for the component to be clickable by accepting either onClick or href.
  • Adjusted styles so that when the label has one letter it is styled similarly to the "icon only" case.
  • Adjusted the background color of the accent variant to match our library on Figma.

image 9

Checklist

  • Check against all themes for compatibility in both light and dark modes
    - [ ] Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@andreadelrio andreadelrio changed the title [EuiBetaBadge] Add more colors and add support for click event [EuiBetaBadge] Add color and size props and support for click event May 14, 2021
@andreadelrio
Copy link
Contributor Author

@miukimiu I know you recently added the lensApp icon to the docs example but since now we're showing more sizes and colors I thought it was getting a bit crowded. Open to suggestions to bring it back.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4798/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks @andreadelrio for improving the EuiBetaBadge. The new size looks great and also the new props to support click events. 🎉

I have a few suggestions.

Also noticed, that when passing a href with color hollow the link get's blue:

Screenshot 2021-05-14 at 14 47 52

This PR can also close #4190. But we also need to allow passing these props to the EuiCard.

Right now we can only pass the following beta badge props:

Screenshot 2021-05-14 at 14 55 43

So maybe we can allow passing all the rest of the beta badge props like betaBadgeProps.

@@ -2,23 +2,36 @@ import React from 'react';

import { EuiBetaBadge, EuiSpacer, EuiTitle } from '../../../../src/components';

const colors = ['hollow', 'accent', 'subdued'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support all the badge colors and also allow to use custom color examples?

Screenshot 2021-05-14 at 14 39 44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I wonder if the beta badge's use cases are better suited for a limited set of colors. Likely @cchaos can weigh in as I think she's the one who proposed adding two more colors only.

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 it could sway either way, keeping it limited may help users notice it more since they tend to mark product features. If we were to change the set to accept the same as regular badges, are they then any different really? I would then consider if there's any real necessity to keep separate components. But I'd suggest starting a more targeted discussion for that and keep this PR scoped to the current changeset.

src-docs/src/views/badge/beta_badge.js Show resolved Hide resolved
@andreadelrio
Copy link
Contributor Author

andreadelrio commented May 18, 2021

@miukimiu I pushed some changes addressing feedback. There's a TS error I'll probably need Greg's (or your) help with.
I added betaBadgeProps to EuiCard and that raised the following question (also related to #4190):

Frame 9

Should EuiBetaBadge "follow" EuiCard's isDisabled state or override it? Right now it overrides it. I'm wondering if we should give EuiBetaBadge its own isDisabled state or if there's a better way to handle this scenario. Another alternative would be to have EuiBetaBadge override EuiCard's isDisabled state only if the badge has a click event associated with it.

@miukimiu
Copy link
Contributor

By default, the EuiBetaBadge should respect the EuiCard's isDisabled state. We don't want to introduce changes to the default card with a EuiBetaBadge because this would impact all these types of cards being used in Kibana.

We just want to give more flexibility to consumers. So in case, consumers want a card with a EuiBetaBadge with a different color, click event, non-disabled state, they can override the default props with the EuiBetaBadgeProps. In the past, this wasn't possible.

Comment on lines 165 to 173
{
'euiBetaBadge--iconOnly': iconType,
},
{
'euiBetaBadge--singleLetter': singleLetter,
},
{
'euiBetaBadge-isClickable': onClick || href,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick suggestion 😉 . All of these can actually be combined in one object

Suggested change
{
'euiBetaBadge--iconOnly': iconType,
},
{
'euiBetaBadge--singleLetter': singleLetter,
},
{
'euiBetaBadge-isClickable': onClick || href,
},
{
'euiBetaBadge--iconOnly': iconType,
'euiBetaBadge--singleLetter': singleLetter,
'euiBetaBadge-isClickable': onClick || href,
},

@cchaos
Copy link
Contributor

cchaos commented May 19, 2021

As a reminder, one of the use-cases for clickable beta badges on cards is to direct users towards license info/upgrade for disabled features. So we want to be sure that it's very easy to have a disabled card with a clickable beta badge.

@andreadelrio
Copy link
Contributor Author

andreadelrio commented May 21, 2021

As a reminder, one of the use-cases for clickable beta badges on cards is to direct users towards license info/upgrade for disabled features. So we want to be sure that it's very easy to have a disabled card with a clickable beta badge.

Keeping this in mind and after discussing with @miukimiu we agreed to go with the EuiBetaBadge following EuiCard's isDisabled state unless the beta badge isClickable. I updated the examples on the "Cards with beta badges" section to show consumers this use case.

image

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

Tested again in different browsers and locally. You just need to do a small change to allow passing a custom CSS class.

I tried to look into the TS error, and this code seems to make it disappear:

const betaBadgeClickEventsProps = betaBadgeProps?.href
      ? {
          ...(betaBadgeProps as React.MouseEventHandler<HTMLAnchorElement>),
        }
      : {
          ...(betaBadgeProps as React.MouseEventHandler<HTMLButtonElement>),
        };

    optionalBetaBadge = (
      <span className="euiCard__betaBadgeWrapper">
        <EuiBetaBadge
          id={optionalBetaBadgeID}
          label={betaBadgeLabel}
          title={betaBadgeTitle}
          tooltipContent={betaBadgeTooltipContent}
          className="euiCard__betaBadge"
          {...betaBadgeClickEventsProps}
        />
      </span>
    );

But I'm not completely sure if this is the right solution, so it's better if @thompsongl takes a look! 😄

@@ -298,6 +303,7 @@ export const EuiCard: FunctionComponent<EuiCardProps> = ({
label={betaBadgeLabel}
title={betaBadgeTitle}
tooltipContent={betaBadgeTooltipContent}
{...betaBadgeProps}
className="euiCard__betaBadge"
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to be able to pass a custom className:

Suggested change
className="euiCard__betaBadge"
className={classNames(
'euiCard__betaBadge',
betaBadgeProps?.className
)}

@thompsongl
Copy link
Contributor

I'd like to propose that we deprecate betaBadgeLabel, betaBadgeTooltipContent, betaBadgeTitle in favor of using betaBadgeProps alone.
The API would be much more clear, and betaBadgeProps?: EuiBetaBadgeProps; resolves the TS error.
It is a breaking change, which is why I think deprecation and the option to specify label, tooltipContent, and title in betaBadgeProps right now is the best approach.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4798/

src/components/card/card.tsx Outdated Show resolved Hide resolved
src/components/badge/beta_badge/beta_badge.tsx Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor

There's an axe test failure, too:

15:12:34 Errors on tp://localhost:9999/#/display/badge
15:13:58 [button-name]: Ensures buttons have discernible text
15:13:58 Help: https://dequeuniversity.com/rules/axe/4.1/button-name?application=axe-puppeteer
15:13:58 Elements:
15:13:58 - .euiBetaBadge-isClickable.euiBetaBadge--iconOnly[title=""]
15:13:58 1 accessibility errors

@thompsongl thompsongl mentioned this pull request May 26, 2021
35 tasks
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4798/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

andreadelrio#15 has a couple TS things I was working on
Otherwise just the additional comments

src/components/card/card.tsx Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4798/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Code changes LGTM!

@thompsongl thompsongl added the deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. label May 28, 2021
@andreadelrio andreadelrio merged commit 911bee4 into elastic:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants