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

[EuiSteps] Remove chroma transparency warning, misc cleanup #7454

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

cee-chen
Copy link
Contributor

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

Summary

Several Kibana devs have reported this chroma.js warning for disabled EuiSteps:

Unfortunately, it looks like #7032 did not fully resolve the console warnings. The usage is coming from here:

color: ${makeHighContrastColor(euiTheme.colors.disabledText)(
euiButtonFillColor(euiThemeContext, 'disabled').backgroundColor
)};

euiButtonFillColor(euiThemeContext, 'disabled').backgroundColor is a color with alpha transparency on it, so we cannot call makeHighContrastColor() on it without this warning.

I've opted to revert disabled step numbers to use the same text & background colors as euiButtonFillColor() entirely, while leaving the step title the same higher contrast color. IMO, this is significantly DRYer approach to take - if we need to increase color contrast, we should do so on buttons as well as steps.

Before After

This PR also DRYs out/cleans up some other CSS, which can be viewed by commit.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
      - [ ] Checked in mobile
  • Docs site QA - N/A
  • Code quality checklist - N/A, CSS only changes
  • 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
    • Updated the Figma library counterpart

- It's creating a `chroma.js` warning that devs are concerned about

- we should really just be reusing the default EuiButton colors anyway, and tweaking those if they have color contrast issues, so that multiple places at once get the updates
- remove multiple `euiButtonFillColor()` calls in favor of just one
- remove CSS nesting

- replace hardcoded 2px with token
@cee-chen cee-chen requested a review from 1Copenut January 9, 2024 19:08
@cee-chen
Copy link
Contributor Author

cee-chen commented Jan 9, 2024

@1Copenut do you mind weighing in on this since you approved #7032? IMO, the step number is less important in terms of contrast than the step title/text. If we still feel it needs higher contrast, I think we should do so in the euiButtonFillColor and propagate that to all buttons instead of just steps.

@cee-chen cee-chen marked this pull request as ready for review January 9, 2024 19:24
@cee-chen cee-chen requested a review from a team as a code owner January 9, 2024 19:24
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I took a look at the view in light and dark themes and ran the axe browser plugin. Zero issues or console output. Contrast ratio on the gray text was 4.78:1 and 5:26(?):1 on light and dark themes respectively.

Clean refactor @cee-chen!

@cee-chen
Copy link
Contributor Author

cee-chen commented Jan 9, 2024

Thanks Trevor!

@cee-chen cee-chen merged commit 01f5bd5 into elastic:main Jan 9, 2024
8 checks passed
@cee-chen cee-chen deleted the steps-cleanup branch January 9, 2024 22:08
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))
cee-chen added a commit to elastic/kibana that referenced this pull request Jan 24, 2024
`v92.0.0-backport.0`⏩ `v92.1.1`

---

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

**Bug fixes**

- Minor `EuiDataGrid` cell performance fixes
([#7465](elastic/eui#7465))

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

- Updated `EuiResizableButton` to allow customizing the `indicator`
style with either `handle` (default) or `border`
([#7455](elastic/eui#7455))
- Enhanced `EuiResizableContainer` to preserve the drag/resize event
when the user's mouse leaves the parent container and re-enters
([#7456](elastic/eui#7456))

**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))

**Accessibility**

- `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to
be more consistent for varying complex data:
([#7448](elastic/eui#7448))
- Headers are now always navigable by arrow key, regardless of whether
the header cells contain interactive content
- Non-expandable cells containing any amount of interactive content now
must be entered via Enter or F2 keypress
  - Expandable cells continue to be toggled via Enter or F2 keypress
- `EuiDataGrid` now provides a direct screen reader hint for Enter key
behavior for expandable & interactive cells
([#7448](elastic/eui#7448))
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
`v92.0.0-backport.0`⏩ `v92.1.1`

---

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

**Bug fixes**

- Minor `EuiDataGrid` cell performance fixes
([elastic#7465](elastic/eui#7465))

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

- Updated `EuiResizableButton` to allow customizing the `indicator`
style with either `handle` (default) or `border`
([elastic#7455](elastic/eui#7455))
- Enhanced `EuiResizableContainer` to preserve the drag/resize event
when the user's mouse leaves the parent container and re-enters
([elastic#7456](elastic/eui#7456))

**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))

**Accessibility**

- `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to
be more consistent for varying complex data:
([elastic#7448](elastic/eui#7448))
- Headers are now always navigable by arrow key, regardless of whether
the header cells contain interactive content
- Non-expandable cells containing any amount of interactive content now
must be entered via Enter or F2 keypress
  - Expandable cells continue to be toggled via Enter or F2 keypress
- `EuiDataGrid` now provides a direct screen reader hint for Enter key
behavior for expandable & interactive cells
([elastic#7448](elastic/eui#7448))
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