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

[EuiTab] Fix prepend/append elements not inheriting selected colors + misc CSS cleanup #6938

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 12, 2023

Summary

fixes #6935

Pre-Emotion conversion (#6311), icons used within the prepend/append props of EuiTab correctly inherited the selected primary color (go to https://eui.elastic.co/v60.0.0/#/navigation/tabs#tabbed-content and click the hydrogen tab).

This PR correctly sets the selected text color of all children of EuiTab, and incidentally cleans up CSS, reducing unnecessary overrides and removing some CSS that isn't doing anything (I recommend following along by commit).

QA

  • Go to https://eui.elastic.co/pr_6938/#/navigation/tabs#tabbed-content
  • Click the Hydrogen tab
  • Confirm the icon changes to blue alongside the tab text
  • Regression testing
    • In the first/top example on the page, confirm the disabled tab with an icon are both the same disabled color
    • In the playground, confirm the different tab sizes are still the same as production

General checklist

  • Checked in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests - mostly just updating snapshots
  • A changelog entry exists and is marked appropriately

cee-chen added 4 commits July 12, 2023 08:54
- Using the `euiTitle()` util gives us too much we don't need (color, font-weight, etc.) - it makes more sense to simply use `euiFontSize` directly

- doing so allows us to set `color` once on the parent button
- focus and hover styles aren't necessary or doing anything - remove them
@cee-chen cee-chen requested a review from breehall July 12, 2023 16:22
@cee-chen cee-chen changed the title [EuiTabs] Fix prepend/append elements not inheriting selected colors + misc CSS cleanup [EuiTab] Fix prepend/append elements not inheriting selected colors + misc CSS cleanup Jul 12, 2023
@cee-chen cee-chen marked this pull request as ready for review July 12, 2023 16:37
@kibanamachine
Copy link

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

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.

✅ This is good to go! Nice solution and CSS clean up as well. Confirmed that the original case is fixed as well as the regression items listed in your description.

@cee-chen
Copy link
Contributor Author

Thanks a million Bree!

@cee-chen cee-chen merged commit 5adce56 into elastic:main Jul 12, 2023
@cee-chen cee-chen deleted the tabs-fix-icon-color branch July 12, 2023 21:15
Ikuni17 pushed a commit to elastic/kibana that referenced this pull request Jul 27, 2023
## Summary

`eui@84.0.0` ⏩ `eui@85.0.1`

## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1)

**Bug fixes**

- Fixed `EuiFilterGroup`'s responsive styles
([#6983](elastic/eui#6983))

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

- Updated `EuiThemeProvider` to set an Emotion theme context that
returns the values of `useEuiTheme()`
([#6913](elastic/eui#6913))
- Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous
size of `m` ([#6928](elastic/eui#6928))
- Added new `s` sizing to `EuiStepsHorizontal`
([#6928](elastic/eui#6928))
- Added `at` and `key` icon glyphs.
([#6934](elastic/eui#6934))
- Added a new `cloneElementWithCss` Emotion utility
([#6939](elastic/eui#6939))
- Updated `EuiPopover` to allow consumer control of all `focusTrapProps`
([#6955](elastic/eui#6955))

**Bug fixes**

- Fixed `EuiDataGrid` height calculation bug when browser zoom levels
are not 100% ([#6895](elastic/eui#6895))
- Fixed `EuiTab` not correctly passing selection color state to
`prepend` and `append` children
([#6938](elastic/eui#6938))
- Fixed `EuiInputPopover` to allow consumer control of its focus trap
via `focusTrapProps` ([#6955](elastic/eui#6955))

**Breaking changes**

- `EuiProvider` will no longer render multiple or duplicate nested
instances of itself. If a nested `EuiProvider` is detected, that
instance will return early without further processing, and will warn if
configured to do so via `setEuiDevProviderWarning`. For nested theming,
use `EuiThemeProvider` instead.
([#6949](elastic/eui#6949))
- Removed `onTrapDeactivation` prop from `EuiPopover`. Use
`focusTrapProps.onDeactivation` instead
([#6955](elastic/eui#6955))

**CSS-in-JS conversions**

- Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed
styles attached to `.euiFilterGroup__popoverPanel`
([#6957](elastic/eui#6957))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## Summary

`eui@84.0.0` ⏩ `eui@85.0.1`

## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1)

**Bug fixes**

- Fixed `EuiFilterGroup`'s responsive styles
([elastic#6983](elastic/eui#6983))

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

- Updated `EuiThemeProvider` to set an Emotion theme context that
returns the values of `useEuiTheme()`
([elastic#6913](elastic/eui#6913))
- Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous
size of `m` ([elastic#6928](elastic/eui#6928))
- Added new `s` sizing to `EuiStepsHorizontal`
([elastic#6928](elastic/eui#6928))
- Added `at` and `key` icon glyphs.
([elastic#6934](elastic/eui#6934))
- Added a new `cloneElementWithCss` Emotion utility
([elastic#6939](elastic/eui#6939))
- Updated `EuiPopover` to allow consumer control of all `focusTrapProps`
([elastic#6955](elastic/eui#6955))

**Bug fixes**

- Fixed `EuiDataGrid` height calculation bug when browser zoom levels
are not 100% ([elastic#6895](elastic/eui#6895))
- Fixed `EuiTab` not correctly passing selection color state to
`prepend` and `append` children
([elastic#6938](elastic/eui#6938))
- Fixed `EuiInputPopover` to allow consumer control of its focus trap
via `focusTrapProps` ([elastic#6955](elastic/eui#6955))

**Breaking changes**

- `EuiProvider` will no longer render multiple or duplicate nested
instances of itself. If a nested `EuiProvider` is detected, that
instance will return early without further processing, and will warn if
configured to do so via `setEuiDevProviderWarning`. For nested theming,
use `EuiThemeProvider` instead.
([elastic#6949](elastic/eui#6949))
- Removed `onTrapDeactivation` prop from `EuiPopover`. Use
`focusTrapProps.onDeactivation` instead
([elastic#6955](elastic/eui#6955))

**CSS-in-JS conversions**

- Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed
styles attached to `.euiFilterGroup__popoverPanel`
([elastic#6957](elastic/eui#6957))

---------

Co-authored-by: Cee Chen <constance.chen@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiTab] Icons on selected tabs don't inherit the primary color as the text does
3 participants