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

[Emotion] Convert EuiBadgeGroup, EuiBetaBadge, and EuiNotificationBadge #6258

Merged
merged 8 commits into from
Sep 23, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 21, 2022

Summary

Follow up to #6224 - converts all remaining Sass in badge-adjacent components to Emotion. I recommend following along by commit and peeking at commit messages for more context.

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Updated the Figma library counterpart

Things to look out for when moving styles

  • Convert side specific padding, margin, and position to -inline and -block (Logical properties)
  • Reduce specificity where possible (usually by reducing class name chaining)
  • All animations or transitions are wrapped in euiCanAnimate
  • Use gap property to add margin between items if using flex
  • Delete any className maps if not being used in Kibana. If used in Kibana, consider converting to a data attribute instead
  • Delete any Amsterdam override .scss files
  • Can any still existing .js files be converted to TS? (e.g., docs files)

- [ ] Anything in the backlog that could be a quick fix for that component?
- [ ] Convert component-specific Sass vars to exported JS versions
- [ ] Convert global Sass vars to JS version like sizing

QA

  • Does the output CSS match the previous CSS / as expected
  • Does the rendered class read as expected
  • shouldRenderCustomStyles() passes with parent component and any nested childProps (e.g. tooltipProps, buttonProps)

- opinionated refactor: Use `inline-flex` and `gap` instead of inline-block and margins

- doing so remove the needs for `euiBadgeGroup__item`
- opinionated refactor: simplify `s` font-size to static rem (since .625 doesn't match the ratio of any current font scales)

- remove isClickable CSS / classNames - it's not actually being used anywhere that I can tell

- add unit tests for badge circle display
- convert file to TS
- improve visuals of clickable examples
+ delete all badge SCSS files

+ update downstream snapshots
@cee-chen
Copy link
Contributor Author

CCing @daveyholler in on this once, since he was pairing/following along as I worked on some of these - figured he might be curious to see the end result :)

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

This looks great @constancecchen! 🎉

I tested in Chrome, Safari, and, Firefox. I also looked into the code and I couldn't find any blocker.

But I have a few code suggestions. 👩🏽‍💻


export default () => (
<div>
<>
{colors.map((item, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having the horizontal spaces between each EuiBetaBadge with a &emsp; what do you think of wrapping the colors.map() with a div with a gap: 0.5rem;?

<div
  css={css`
    display: flex;
    flex-direction: column;
    gap: 0.5rem;
  `}
>
  {colors.map((item, index) => (
    <div key={index}>
      <EuiBetaBadge />
      <EuiBetaBadge />
      <EuiBetaBadge />
      <EuiSpacer size="s" />
    </div>
  ))}
</div>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this will work quite the way you expect because the gap will only apply to the immediate child divs and not to the grandchild EuiBetaBadges (meaning there will no longer be a slight horizontal gap).

To get this working with gap and without the &emsp;s I think we should use a grid (which I'm happy to play around with!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +57 to +61
<EuiBetaBadge
label="Lens"
iconType="lensApp"
onClick={() => alert('Goes to Lens')}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed in CodeSandbox that still has the .scss this example doesn't get a circular EuiBetaBadge:

Screenshot 2022-09-22 at 11 54 24

If I copy the new styles and add them in Chrome developer tool the beta badge doesn't get circular. So not sure if this issue keeps persisting with the new styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CodeSandbox doesn't update until our releases, so I can't say for sure. If the staging/local example works, I think this should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the current version (https://elastic.github.io/eui/#/display/badge#beta-badge-type) it works in staging/local but it looks different in CodeSandbox. The beta badge with the lens icon doesn't get circular.

So I'm not sure this problem will persist. But we can take a look after this PR is merged and there's a new release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's wait until the release, it's a little too hard for me to debug right now with the different styles. If it persists after release I think I may have a solution however!

Comment on lines 21 to 24
const textColor =
colorMode === 'DARK' ? euiTheme.colors.ghost : euiTheme.colors.ink;
const invertedTextColor =
colorMode === 'DARK' ? euiTheme.colors.ink : euiTheme.colors.ghost;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be less manual and we use instead a function to determine whether or not to use light or dark text against the given background color? We can use the isColorDark utility.

const textColor = (backgroundColor: any) => {
    return isColorDark(...hexToRgb(backgroundColor))
      ? euiTheme.colors.ghost
      : euiTheme.colors.ink;
  };

hollow: css`
      background-color: ${euiTheme.colors.emptyShade};
      color: ${textColor(euiTheme.colors.emptyShade)};
 `,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Personally I don't find a function significantly more useful/easier to read than a basic set of ternaries with the current set of color options, but it doesn't bother me either way and I can see the potential use if we add more colors in the future

Copy link
Contributor Author

@cee-chen cee-chen Sep 22, 2022

Choose a reason for hiding this comment

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

e3ddcd0

Also it looks like we still need a ternary for the focus ring outline, since that doesn't really have the concept of a background color.

But overall on second thought I do think this DRY's out things a good amount. Thanks Elizabet!!

src/components/badge/beta_badge/beta_badge.styles.ts Outdated Show resolved Hide resolved
@cee-chen cee-chen enabled auto-merge (squash) September 22, 2022 17:04
@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit 08c4ac4 into elastic:main Sep 23, 2022
@cee-chen cee-chen deleted the emotion/badge branch September 23, 2022 16:03
jbudz pushed a commit to elastic/kibana that referenced this pull request Nov 18, 2022
## Summary
`eui@67.1.8` ⏩ `eui@70.2.2`

⚠️ Note: This upgrade contains breaking changes to `EuiFlexGroup` and
`EuiFlexGrid`, primarily around switching margins and negative margins
to `gap`. Please do a quick QA pass of your app to scan for any issues.
We're happy to help resolve minor fixes, or potentially follow up after
PR merges. You can find us over in #eui!

## [`70.2.4`](https://github.com/elastic/eui/tree/v70.2.4)

**Bug fixes**

- Fixed visual bug in nested `EuiFlexGroup`s, where the parent
`EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not
([#6381](elastic/eui#6381))

## [`70.2.3`](https://github.com/elastic/eui/tree/v70.2.3)

**Bug fixes**

- Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex`
CSS gap change ([#6380](elastic/eui#6380))

## [`70.2.2`](https://github.com/elastic/eui/tree/v70.2.2)

- `EuiButton` now accepts `minWidth={false}`
([#6373](elastic/eui#6373))

**Bug fixes**

- `EuiButton` no longer outputs unnecessary inline styles for
`minWidth={0}` or `minWidth={false}`
([#6373](elastic/eui#6373))
- `EuiFacetButton` no longer reports type issues when passing props
accepted by `EuiButton`
([#6373](elastic/eui#6373))
- Fixed the shadow sizes of `.eui-yScrollWithShadows` and
`.eui-xScrollWithShadows`
([#6374](elastic/eui#6374))


## [`70.2.1`](https://github.com/elastic/eui/tree/v70.2.1)

**Bug fixes**

- Re-fixed `EuiPageSection` not correctly merging `contentProps.css`
([#6365](elastic/eui#6365))
- Fixed `EuiTab` not defaulting to size `m`
([#6366](elastic/eui#6366))

## [`70.2.0`](https://github.com/elastic/eui/tree/v70.2.0)

- Added a keyboard shortcuts popover to `EuiDataGrid`'s toolbar. This
can be visually hidden via `toolbarVisibility.showKeyboardShortcuts`,
but will always remain accessible to keyboard and screen reader users.
([#6036](elastic/eui#6036))
- `EuiScreenReaderOnly`'s `showOnFocus` prop now also shows on focus
within its children ([#6036](elastic/eui#6036))
- Added `onFocus` prop callback to `EuiSuperDatePicker`
([#6320](elastic/eui#6320))

**Bug fixes**

- Fixed `EuiSelectable` to ensure the full options list is re-displayed
when the search bar is controlled and cleared using `searchProps.value`
([#6317](elastic/eui#6317))
- Fixed incorrect padding on `xl`-sized `EuiTabs`
([#6336](elastic/eui#6336))
- Fixed `EuiCard` not correctly merging `css` on its child `icon`s
([#6341](elastic/eui#6341))
- Fixed `EuiCheckableCard` not setting `css` on the correct DOM node
([#6341](elastic/eui#6341))
- Fixed a webkit rendering issue with `EuiModal`s containing
`EuiBasicTable`s tall enough to scroll
([#6343](elastic/eui#6343))
- Fixed bug in `to_initials` that truncates custom initials
([#6346](elastic/eui#6346))
- Fix bug in `EuiCard` where layout breaks when `horizontal` and
`selectable` are both passed
([#6348](elastic/eui#6348))

## [`70.1.0`](https://github.com/elastic/eui/tree/v70.1.0)

- Added the `hint` prop to the `<EuiSearchBar />`. This prop lets the
consumer render a hint below the search bar that will be displayed on
focus. ([#6319](elastic/eui#6319))
- Added the `hasDragDrop` prop to `EuiPopover`. Use this prop if your
popover contains `EuiDragDropContext`.
([#6329](elastic/eui#6329))

**Bug fixes**

- Fixed `EuiButton`'s cursor style when the button is disabled
([#6323](elastic/eui#6323))
- Fixed `EuiPageTemplate` not recognizing child
`EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props
([#6324](elastic/eui#6324))
- Fixed `EuiBetaBadge` to always respect its `anchorProps` values,
including when there is no tooltip content
([#6326](elastic/eui#6326))
- Temporarily patched `EuiModal` to not cause scroll-jumping issues on
modal open ([#6327](elastic/eui#6327))
- Fixed buggy drag & drop behavior within `EuiDataGrid`'s columns &
sorting toolbar popovers
([#6329](elastic/eui#6329))
- Fixed `EuiButton` not correctly passing `textProps` for children
inside fragments or i18n components
([#6332](elastic/eui#6332))
- Fixed `EuiButton` not correctly respecting `minWidth={0}`
([#6332](elastic/eui#6332))

**CSS-in-JS conversions**

- Converted `EuiTabs` to Emotion
([#6311](elastic/eui#6311))

## [`70.0.0`](https://github.com/elastic/eui/tree/v70.0.0)

- Added the `enabled` option to the `<EuiInMemoryTable />`
`executeQueryOptions` prop. This option prevents the Query from being
executed when controlled by the consumer.
([#6284](elastic/eui#6284))

**Bug fixes**

- Fixed `EuiOverlayMask` to set a
`[data-relative-to-header=above|below]` attribute to replace the
`--aboveHeader` and `--belowHeader` classNames removed in its Emotion
conversion ([#6289](elastic/eui#6289))
- Fixed `EuiHeader` CSS using removed `EuiOverlayMask` class modifiers
([#6293](elastic/eui#6293))
- Fixed `EuiToolTip` not respecting reduced motion preferences
([#6295](elastic/eui#6295))
- Fixed a bug with `EuiTour` where passing any `panelProps` would cause
the beacon to disappear
([#6298](elastic/eui#6298))

**Breaking changes**

- `@emotion/css` is now a required peer dependency, alongside
`@emotion/react` ([#6288](elastic/eui#6288))
- `@emotion/cache` is no longer required peer dependency, although your
project must still use it if setting custom cache/injection locations
([#6288](elastic/eui#6288))

**CSS-in-JS conversions**

- Converted `EuiCode` and `EuiCodeBlock` to Emotion; Removed
`euiCodeSyntaxTokens` Sass mixin and `$euiCodeBlockPaddingModifiers`;
([#6263](elastic/eui#6263))
- Converted `EuiResizableContainer` and `EuiResizablePanel` to Emotion
([#6287](elastic/eui#6287))

## [`69.0.0`](https://github.com/elastic/eui/tree/v69.0.0)

- Added support for `fullWidth` prop on EuiForm, which will be the
default for all rows/controls within
([#6229](elastic/eui#6229))
- Added support for `onResizeStart` and `onResizeEnd` callbacks to
`EuiResizableContainer`
([#6236](elastic/eui#6236))
- Added optional case sensitive option matching to `EuiComboBox` with
the `isCaseSensitive` prop
([#6268](elastic/eui#6268))
- `EuiFlexItem` now supports `grow={0}`
([#6270](elastic/eui#6270))
- Added the `alignItems` prop to `EuiFlexGrid`
([#6281](elastic/eui#6281))
- Added `filter`, `filterExclude`, `filterIgnore`, `filterInclude`,
`indexTemporary`, `infinity`, `sortAscending`, and `sortDescending`
glyphs to `EuiIcon` ([#6282](elastic/eui#6282))

**Bug fixes**

- Fixed `EuiTextProps` to show the `color` type option `inherit` as
default ([#6267](elastic/eui#6267))
- `EuiFlexGroup` now correctly respects `gutterSize` when responsive
([#6270](elastic/eui#6270))
- Fixed the last breadcrumb in `EuiBreadcrumbs`'s `breadcrumbs` array
not respecting `truncate` overrides
([#6280](elastic/eui#6280))

**Breaking changes**

- `EuiFlexGrid` no longer supports `columns={0}`. Use `EuiFlexGroup`
instead for normal flex display
([#6270](elastic/eui#6270))
- `EuiFlexGrid` now uses modern `display: grid` CSS
([#6270](elastic/eui#6270))
- `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` now use modern `gap`
CSS instead of margins and negative margins
([#6270](elastic/eui#6270))
- `EuiFlexGroup` no longer applies responsive styles to `column` or
`columnReverse` directions
([#6270](elastic/eui#6270))

**CSS-in-JS conversions**

- Converted `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` to Emotion
([#6270](elastic/eui#6270))

## [`68.0.0`](https://github.com/elastic/eui/tree/v68.0.0)

- Added `beta` glyph to `EuiIcon`
([#6250](elastic/eui#6250))
- Added `launch` and `spaces` glyphs to `EuiIcon`
([#6260](elastic/eui#6260))
- Added the `fallbackDestination` prop to `EuiSkipLink`, which accepts a
string of query selectors to fall back to if the `destinationId` does
not have a valid target. Defaults to `main`
([#6261](elastic/eui#6261))
- `EuiSkipLink` is now always an `a` tag to ensure that it is always
placed within screen reader link menus.
([#6261](elastic/eui#6261))

**Bug fixes**

- Fixed `EuiSuperDatePicker` not correctly merging passed `className`s
([#6253](elastic/eui#6253))
- Fixed `EuiColorStops` not correctly merging in passed
`data-test-subj`s, `style`s, or `...rest`
([#6255](elastic/eui#6255))
- Fixed `EuiResizablePanel` incorrectly passing `style` to the wrapper
instead of the panel. Use `wrapperProps.style` to pass styles to the
wrapper. ([#6255](elastic/eui#6255))
- Fixed custom `onClick`s passed to `EuiSkipLink` overriding
`overrideLinkBehavior`
([#6261](elastic/eui#6261))

**Breaking changes**

- Removed `inherit` and `ghost` color from `EuiListGroupItem`
([#6207](elastic/eui#6207))
- Changed default color to `text` instead of `inherit`
([#6207](elastic/eui#6207))

**CSS-in-JS conversions**

- Converted `EuiListGroup` and `EuiListGroupItem` to Emotion; Removed
`$euiListGroupGutterTypes`, `$euiListGroupItemColorTypes` and
`$euiListGroupItemSizeTypes`;
([#6207](elastic/eui#6207))
- Converted `EuiBadgeGroup` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiBetaBadge` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiNotificationBadge` to Emotion
([#6258](elastic/eui#6258))

Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants