Beta Badges #705

cchaos · 2018-04-26T15:05:07Z

Fixes #682

This adds a new component called EuiBetaBadge that only accepts a label and description. It will only add a tooltip if the description prop exists.

Can be used on cards

And Key Pad Menu Items

This will truncate to only one letter and ensure the full label is supplied to the tooltip as a title or if there is no tooltip (no description) it will add the title just as a norma title attribute.

cc @nreese

nreese

This looks really nice - thank you

nreese · 2018-04-26T15:20:06Z

src-docs/src/views/badge/badge_example.js

+        </p>
+        <p>
+          They can also be used in conjunction
+          with <EuiLink href="/#/display/card">EuiCards</EuiLink> and EuiKeyPadMenuItems.


Can you also make EuiKeyPadMenuItems a link to key pad menu?

Yeah, I meant to do that. Thanks.

snide

These look great. Minor comments in the API naming, but otherwise they look hot.

I think it'd be nice if you could pass an icon specifically to the keypad one. I imagine a "beaker" icon or something might read better initially other than the "B" or "L".

This is a really nice example though of all our components coming together individually.

snide · 2018-04-26T16:42:33Z

src/components/badge/beta_badge/_beta_badge.scss

+
+  font-size: $euiSizeM;
+  font-weight: $euiFontWeightBold;
+  letter-spacing: .05em;


This is great. Should we make some spacing vars in typography? $euiSpacingS/M/L...etc?

This is a very rare case, mainly only needed for bold and uppercase type. I don't think it's necessary to make variables as it wouldn't really be an override.

Though looking at this I did just realize that the font-size isn't using a font-size variable like it should. I'll change that.

snide · 2018-04-26T16:46:32Z

src/components/badge/beta_badge/_beta_badge.scss

+  text-transform: uppercase;
+  line-height: $euiSizeL;
+  text-align: center;
+  white-space: nowrap;


Probably don't need it, but you might want to account for ellipsis or something because of the nowrap. We do something similar in the badges. This can just be an issue reminder for us, hopefully these are short.

Yeah, I can add a max-width for card implementations. But you're right that these should not contain more than 1-2 words ever.

snide · 2018-04-26T16:50:26Z

src/components/badge/beta_badge/beta_badge.js

+    return (
+      <EuiToolTip
+        position={tooltipPosition}
+        content={description}


Normally when we've done this kind of passthru we've tried to keep the naming similar... aka EuiIcon's type prop becomes iconType when passed through to another component. Just helps better understand the API a bit. Might want to consider doing that here. For example, I see you're doing it in the line above will tooltipPosition

snide · 2018-04-26T16:50:57Z

src/components/card/_card.scss

+      left: 50%;
+      transform: translateX(-50%);
+      z-index: 3; // get above abs positioned image
+      min-width: 40%; /* 2 */


Nice touch here.

snide · 2018-04-26T16:53:45Z

src/components/key_pad_menu/_key_pad_menu.scss

+          width: $euiSizeL;
+          padding: 0 8px;  /* 2 */
+          overflow: hidden;  /* 2 */
+          letter-spacing: 3rem;  /* 2 */


Same comment on variabalizing this if possible.

Yeah, and same response to this as well, super-specific use case here where I'm just separating the letters enough to only show 1 letter. Doesn't make sense for it to be a variable.

snide · 2018-04-26T16:57:04Z

src/components/key_pad_menu/key_pad_menu_item.js

  <div className="euiKeyPadMenuItem__inner">
+    {betaLabel &&
+      <span className="euiKeyPadMenuItem__betaBadgeWrapper">
+        <EuiBetaBadge label={betaLabel} title={betaLabel} description={betaDescription} className="euiKeyPadMenuItem__betaBadge" />


Similar comment about passthru naming. betaLabel should maybe be betaBadgeTitle, description becomes toolTipContent...etc.

Just thinking out loud here. I know we're now three removed from the tooltip, but I think it might make it easier to understand. What do you think?

I think it would have to be betaBadgeTooltipContent or else it could get confusing what the tooltip will be for. Does that then start getting down a slippery slope of giant naming conventions?

You're right, and I think we should be OK with being overly explicit. Think of someone with no knowledge of the component on some random page. At least they'll understand what is what just by reading it, otherwise you might assume the description is tied to the menu item directly.

It's pretty rare to have a three deep passthru like this. Even more so for one that isn't initially visible during render.

What would you do for naming when a prop can be used in multiple ways? For instance, the title prop will either be the EuiTooltip title={title} or <span title={title}>.

Dealers choice! I'd probably use a unique name if it's an either / or. Whatever you think is best.

- Fixing use of font-size variables - Adding example of badge inline with title - Max-widthed the card version - Now always using the label as the tooltip title or title attr if title is not supplied - Fixing naming conventions to better suit imported components - Allowing icon only badges

cchaos · 2018-04-26T18:43:27Z

@snide Ok I addressed all your comments.

Fixing use of font-size variables
Adding example of badge inline with title
Max-widthed the card version
Now always using the label as the tooltip title or title attr if title is not supplied
Fixing naming conventions to better suit imported components
Allowing icon only badges

snide

👍 🙌

cchaos added the feature request label Apr 26, 2018

cchaos requested a review from snide April 26, 2018 15:05

nreese approved these changes Apr 26, 2018

View reviewed changes

snide reviewed Apr 26, 2018

View reviewed changes

cchaos added 2 commits April 26, 2018 13:03

Added beta badges to cards and keypad menu items

c20b97a

Adding link

3e60fc9

cchaos force-pushed the beta-label branch from 3dece54 to 3e60fc9 Compare April 26, 2018 17:03

cchaos added 2 commits April 26, 2018 13:05

changelog

8ef6bcb

snide approved these changes Apr 26, 2018

View reviewed changes

cchaos merged commit 016ec92 into elastic:master Apr 26, 2018

cchaos deleted the beta-label branch April 26, 2018 19:40

cchaos mentioned this pull request Apr 26, 2018

[Add Data] Design input for Beta modules elastic/kibana#17679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beta Badges #705

Beta Badges #705

cchaos commented Apr 26, 2018 •

edited

Loading

nreese left a comment

nreese Apr 26, 2018

cchaos Apr 26, 2018

snide left a comment

snide Apr 26, 2018

cchaos Apr 26, 2018

snide Apr 26, 2018

cchaos Apr 26, 2018

snide Apr 26, 2018

snide Apr 26, 2018

snide Apr 26, 2018

cchaos Apr 26, 2018

snide Apr 26, 2018

cchaos Apr 26, 2018

snide Apr 26, 2018

cchaos Apr 26, 2018

snide Apr 26, 2018

cchaos commented Apr 26, 2018

snide left a comment

Beta Badges #705

Beta Badges #705

Conversation

cchaos commented Apr 26, 2018 • edited Loading

Can be used on cards

And Key Pad Menu Items

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Apr 26, 2018

snide left a comment

Choose a reason for hiding this comment

cchaos commented Apr 26, 2018 •

edited

Loading