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

[Emotion] Add new focus.transparency and focus.backgroundColor theme tokens #6984

Merged
merged 4 commits into from
Jul 24, 2023
Merged

[Emotion] Add new focus.transparency and focus.backgroundColor theme tokens #6984

merged 4 commits into from
Jul 24, 2023

Conversation

cee-chen
Copy link
Member

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

Summary

  • Adds focus.transparency (mostly just needed for focus.backgroundColor) and focus.backgroundColor

    • ⚠️ Note: an euiFocusBackground Sass mixin exists (link). This mixin allows customizing the background color used (defaulting to primary). I did not convert this mixin to a CSS-in-JS util, and opted to go this route (theme tokens) instead. I did this for several reasons:
    • Kibana has multiple usages of basic $euiFocusBackgroundColor usages (which uses the default primary color) and has 0 usages of custom focus background colors. Based on this, consumers will want an easier-to-grab theme token vs. a util they have to specifically import
    • EUI itself only has 2 usages of custom focus background colors, and honestly, those usages don't need a specific mixin. They can simply grab the transparentize util (which does all the heavy lifting) and then the focus.transparency token at that point.

  • Updated various comments in the focus states files, including removing a hypothetical outline token (our theme tokens already suffice for building this CSS, we have an euiOutline() which essentially already does this and more, and there are zero consumer usages of outline-related CSS)

QA

General checklist

  • Checked in both light and dark modes
  • [ ] Added documentation - I don't see any docs listed for the focus ring tokens, so going to skip any specific docs (other than the above QA links) for this
  • [ ] Added or updated jest and cypress tests - Caroline and Greg apparently did not write any tests to snapshot EUI theme output, so there's no tests to update here
  • Updated the Figma library counterpart - renamed $euiFocusBackgroundColor to just Focus background color
  • A changelog entry exists and is marked appropriately

@cee-chen
Add focus.transparency and focus.backgroundColor to EUI theme
d5ae338
@cee-chen
Update focus states comments
cd87d3f 
- remove unused `outline` var, unlikely we'll use this at any point

- removed TODO esque comment
@cee-chen
changelog
a1c5b59
@cee-chen
Copy link
Member Author

This PR does not convert any existing components that use the existing euiFocusBackground mixin or $euiFocusBackgroundColor variables (link to uses) - I wanted to keep this changelog separate and the PR itself atomic/easier to review and discuss certain choices made.

I'll be converting EuiFilterSelectItem shortly (which does require using the new focus background token).

@kibanamachine
Copy link

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

@cee-chen cee-chen requested a review from breehall July 24, 2023 15:50
@cee-chen cee-chen marked this pull request as ready for review July 24, 2023 15:50
@cee-chen
Copy link
Member Author

CI failures are unrelated to this PR and are already failing in main. Please ignore them for now :)

breehall
breehall approved these changes Jul 24, 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.

Given the fact that there are multiple usages of $euiFocusBackgroundColor in Kibana, I think this is definitely the correct approach. When the day comes where we have no Sass in our library, will our deprecation plan be to just remove the mixin and replace usages with the token?

Also thank you for adding additional comments to the styling. Our Sass can be difficult to work with in some places 😅

@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

When the day comes where we have no Sass in our library, will our deprecation plan be to just remove the mixin and replace usages with the token?

Yep, that's correct! Thanks Bree!

@cee-chen cee-chen enabled auto-merge (squash) July 24, 2023 17:41
@cee-chen cee-chen merged commit 8945de5 into elastic:main Jul 24, 2023
2 checks passed
@cee-chen cee-chen deleted the emotion/focus-tokens branch July 24, 2023 17:50
@breehall breehall mentioned this pull request Jul 27, 2023
Merged
mistic pushed a commit to elastic/kibana that referenced this pull request Aug 3, 2023
@breehall @cee-chen @kibanamachine
Upgrade EUI to v85.1.0 (#162660)
68fec4b 
`85.0.0` ➡️ `85.1.0`

⚠️ The biggest change in this PR by far is the **removal** of several
`EuiFilterSelectItem` CSS classes and styles associated with those
classes. EUI has previously exported component-specific CSS classes for
usage such as `.euiFilterSelect__items`,
`.euiFilterGroup__popoverPanel`, or `.euiAccordionForm`.

❌ **As of our move to CSS-in-JS, we are actively moving away from this
architecture**. The goal is to either provide either a component or prop
directly to you instead of a CSS class. As a reminder, our classNames
are not guaranteed APIs and are subject to change without warning -
should you need to tweak or customize EUI's styling, we strongly
recommend passing in your own `className`.

💬 Moving forward, EUI will attempted to convert any usages of removed
CSS classes and their direct usages in Kibana for you. In most cases,
we'll hopefully be able to take the correct path of using a component or
prop instead. In cases where we cannot or significant/complex changes
are required, we may temporarily convert the CSS to a static CSS-in-JS
usage instead and add a TODO asking the relevant team to address this in
their own time (for example, the deprecation of `EuiFilterSelectItem`
and its conversion to `EuiSelectable`).

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

- Updated `EuiComboBox`'s `options` to accept `option.append` and
`option.prepend` props
([#6953](elastic/eui#6953))
- Updated deprecated `.substr()` usages to `.substring()`
([#6954](elastic/eui#6954))
- Updated `EuiInlineEdit`'s read mode button to include a title tooltip,
increasing readability of truncated text
([#6966](elastic/eui#6966))

**Bug fixes**

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

**Deprecations**

- Deprecated `EuiFilterSelectItem`; Use `EuiSelectable` instead
([#6982](elastic/eui#6982))

**CSS-in-JS conversions**

- Converted `EuiFilterSelectItem` to Emotion
([#6982](elastic/eui#6982))
- Removed `.euiFilterSelect__items` CSS; Use `EuiSelectable` instead
([#6982](elastic/eui#6982))
- Removed `.euiFilterSelect__note` and `.euiFilterSelect__noteContent`
CSS; Use `EuiSelectableMessage` instead
([#6982](elastic/eui#6982))
- Added `focus.transparency` and `focus.backgroundColor` theme tokens
([#6984](elastic/eui#6984))

---------

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.
3 participants
@cee-chen @kibanamachine @breehall