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

[EuiButton] Fix overridden (instead of merged) className #6887

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jun 29, 2023

Summary

This bug was introduced during Caroline's Emotion conversion of EuiButton (which was merged in without thorough QA after she left). At least I presume it's a bug and not intentional, no other component's className behaves this way.

(Bug discovered during shouldRenderCustomStyles work, pulling it out to its own PR for easy review and a separate changelog)

QA

General checklist

@cee-chen cee-chen force-pushed the fix-button-classname branch from a735efc to e320ee5 Compare June 29, 2023 05:40
@cee-chen cee-chen requested a review from a team June 29, 2023 05:40
@kibanamachine
Copy link

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

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

It's very interesting that this slid under the radar! Great catch and fix.

❓ Will this impact many snapshots in Kibana since we're adding the euiButton class name back into the mix?

Comment on lines -116 to -117
className="euiButton"
ref={buttonRef}
Copy link
Contributor

Choose a reason for hiding this comment

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

Super interesting. I never noticed this!

@cee-chen
Copy link
Contributor Author

❓ Will this impact many snapshots in Kibana since we're adding the euiButton class name back into the mix?

Haha yup, I'm pretty sure it will 💀 Ah well, it do be that way. We're doing a bunch of Emotion conversions which affect snapshots anyway, so might as well get it fixed

@cee-chen cee-chen merged commit 1f8e4db into elastic:main Jun 29, 2023
@cee-chen cee-chen deleted the fix-button-classname branch June 29, 2023 21:37
1Copenut added a commit to elastic/kibana that referenced this pull request Jul 11, 2023
`eui@83.0.0` ⏩ `83.1.0`

---

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

- Added `placeholder` prop to `EuiInlineEdit`
([#6883](elastic/eui#6883))
- Added `sparkles` glyph to `EuiIcon`
([#6898](elastic/eui#6898))

**Bug fixes**

- Fixed Safari-only bug for single-line row `EuiDataGrid`s, where cell
actions on hover would overlap instead of pushing content to the left
([#6881](elastic/eui#6881))
- Fixed `EuiButton` not correctly merging in passed `className`s with
its base `.euiButton` class
([#6887](elastic/eui#6887))
- Fixed `EuiIcon` not correctly passing the `style` prop custom `img`
icons ([#6888](elastic/eui#6888))
- Fixed multiple components with child props (e.g. `buttonProps`,
`iconProps`, etc.) unsetting EUI's Emotion styling if custom `css` was
passed to the child props object
([#6896](elastic/eui#6896))

**CSS-in-JS conversions**

- Converted `EuiHeader` and `EuiHeaderLogo` to Emotion
([#6878](elastic/eui#6878))
- Removed Sass variables `$euiHeaderDarkBackgroundColor`,
`$euiHeaderBorderColor`, and `$euiHeaderBreadcrumbColor`
([#6878](elastic/eui#6878))
- Removed Sass mixin `@euiHeaderDarkTheme`
([#6878](elastic/eui#6878))
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