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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jul 12, 2023
Merged

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

merged 5 commits into from
Jul 12, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jul 12, 2023 •
edited
Loading

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
Fix EuiTab icons not inheriting the selected color
fb8c61c
@cee-chen
Clean up tab CSS to reduce unnecessary color setting/overrides
c501df3 
- 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
@cee-chen
More CSS cleanup
bd0183d 
- focus and hover styles aren't necessary or doing anything - remove them
@cee-chen
Update snapshots
4552434
@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/

breehall
breehall approved these changes Jul 12, 2023
View reviewed changes
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
Member Author

Thanks a million Bree!

@cee-chen cee-chen merged commit 5adce56 into elastic:main Jul 12, 2023
2 checks passed
@cee-chen cee-chen deleted the tabs-fix-icon-color branch July 12, 2023 21:15
@tkajtoch tkajtoch mentioned this pull request Jul 19, 2023
Merged
Ikuni17 pushed a commit to elastic/kibana that referenced this pull request Jul 27, 2023
@tkajtoch @cee-chen @kibanamachine
Upgrade @elastic/eui to 85.0.1 (#162209)
1c42ee9 
## 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>
adcoelho pushed a commit to adcoelho/kibana that referenced this pull request Jul 28, 2023
@tkajtoch @cee-chen
@kibanamachine @adcoelho
Upgrade @elastic/eui to 85.0.1 (elastic#162209)
ed4d227 
## 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>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
@tkajtoch @cee-chen @kibanamachine
Upgrade @elastic/eui to 85.0.1 (elastic#162209)
5f2e2ba 
## 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
Reviewers

@breehall breehall breehall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
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
@cee-chen @kibanamachine @breehall