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

[EuiFilterButton] DOM text wrapper cleanup #7444

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 3, 2024

Summary

In #7369 / bb8cca0, I added the ability to disable EuiButtonEmpty's text wrapper DOM node via textProps={false}.

Since EuiFilterButton already adds its own custom text wrapper <span> element, we can clean up/go down from two <span> text wrappers to just one, by using the above API.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A, tech debt
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

- since EuiFilterButton already renders its own text wrapper, we're just removing an extra DOM node by removing the default EuiButtonEmpty one

+ using margin-right: auto to maintain the right-aligned arrow
- mark as deprecation to match other EuiPopover DOM cleanup
the min-width CSS was previously only applying for buttons with notifications, due to its `span` wrapper having `display: flex` set.

Now that the extra span wrapper is gone, text content is always rendered under `display: flex`, so we need to add a new CSS modifier to get it rendering the way it is in production
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen marked this pull request as ready for review January 3, 2024 18:52
@cee-chen cee-chen requested a review from a team as a code owner January 3, 2024 18:52
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@cee-chen
Copy link
Member Author

cee-chen commented Jan 5, 2024

Thanks Tomasz!!

@cee-chen cee-chen merged commit e41ca57 into elastic:main Jan 5, 2024
8 checks passed
@cee-chen cee-chen deleted the filter-button-cleanup branch January 5, 2024 16:57
cee-chen added a commit to elastic/kibana that referenced this pull request Jan 10, 2024
`v91.3.1`⏩`v92.0.0-backport.0`

---

##
[`v92.0.0-backport.0`](https://github.com/elastic/eui/releases/v92.0.0-backport.0)

**This is a backport release only intended for use by Kibana.**

**Bug fixes**

- Fixed an `EuiTreeView` JSX Typescript error
([#7452](elastic/eui#7452))
- Fixed a color console warning being generated by disabled `EuiStep`s
([#7454](elastic/eui#7454))


## [`v92.0.0`](https://github.com/elastic/eui/releases/v92.0.0)

- Updated generic types of `EuiBasicTable`, `EuiInMemoryTable` and
`EuiSearchBar.Query.execute` to add `extends object` constraint
([#7340](elastic/eui#7340))
- This change should have no impact on your applications since the
updated types only affect properties that exclusively accept object
values.
- Added a new `EuiFlyoutResizable` component
([#7439](elastic/eui#7439))
- Updated `EuiTextArea` to accept `isClearable` and `icon` as props
([#7449](elastic/eui#7449))

**Bug fixes**

- `EuiRange`/`EuiDualRange`'s track ticks & highlights now update their
positions on resize ([#7442](elastic/eui#7442))

**Deprecations**

- Updated `EuiFilterButton` to remove the second
`.euiFilterButton__textShift` span wrapper. Target
`.euiFilterButton__text` instead
([#7444](elastic/eui#7444))

**Breaking changes**

- Removed deprecated `EuiNotificationEvent`. We recommend copying the
component to your application if necessary
([#7434](elastic/eui#7434))
- Removed deprecated `EuiControlBar`. We recommend using `EuiBottomBar`
instead ([#7435](elastic/eui#7435))
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
`v91.3.1`⏩`v92.0.0-backport.0`

---

##
[`v92.0.0-backport.0`](https://github.com/elastic/eui/releases/v92.0.0-backport.0)

**This is a backport release only intended for use by Kibana.**

**Bug fixes**

- Fixed an `EuiTreeView` JSX Typescript error
([elastic#7452](elastic/eui#7452))
- Fixed a color console warning being generated by disabled `EuiStep`s
([elastic#7454](elastic/eui#7454))


## [`v92.0.0`](https://github.com/elastic/eui/releases/v92.0.0)

- Updated generic types of `EuiBasicTable`, `EuiInMemoryTable` and
`EuiSearchBar.Query.execute` to add `extends object` constraint
([elastic#7340](elastic/eui#7340))
- This change should have no impact on your applications since the
updated types only affect properties that exclusively accept object
values.
- Added a new `EuiFlyoutResizable` component
([elastic#7439](elastic/eui#7439))
- Updated `EuiTextArea` to accept `isClearable` and `icon` as props
([elastic#7449](elastic/eui#7449))

**Bug fixes**

- `EuiRange`/`EuiDualRange`'s track ticks & highlights now update their
positions on resize ([elastic#7442](elastic/eui#7442))

**Deprecations**

- Updated `EuiFilterButton` to remove the second
`.euiFilterButton__textShift` span wrapper. Target
`.euiFilterButton__text` instead
([elastic#7444](elastic/eui#7444))

**Breaking changes**

- Removed deprecated `EuiNotificationEvent`. We recommend copying the
component to your application if necessary
([elastic#7434](elastic/eui#7434))
- Removed deprecated `EuiControlBar`. We recommend using `EuiBottomBar`
instead ([elastic#7435](elastic/eui#7435))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants