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

[Emotion] Convert EuiFieldText #7770

Merged
merged 19 commits into from
May 20, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented May 17, 2024

Summary

This PR converts the EuiFieldText component to Emotion + all the form mixins required for its styles, and updates several downstream components that dogfood EuiFieldText (EuiColorPicker and EuiDatePicker primarily) to look/work as before.

This PR is merging into a feature branch. I want to do way more cleanup, particularly around EuiFormControlLayout, and I suspect there's a lot more DRYing out to be done - but this PR is already getting pretty unwieldy (lots of moving parts, small tweaks), so I picked a spot to stop once everything looked mostly non-broken 😅

As always, I strongly recommend following along by commit, but here's a high level TL;DR of changes:

  • EuiFieldText has been converted to Emotion, which requires converting multiple Sass mixins to JS equivalents
  • Removed euiFormControlSize() JS mixin in favor of a new euiFormControlStyles(), which outputs an object of multiple modifiers/states for reuse
  • Icon padding affordance is now handled via CSS variables (--euiFormControlLeftIconsCount and --euiFormControlRightIconsCount respectively) set dynamically via inline styles, rather than static classNames
  • Components that were dogfooding EuiFieldText and reusing its classNames for styling had to be updated. For example, EuiDatePicker was updated to dogfood the EuiFieldText component directly instead of an <input type="text" />

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
  • Code quality checklist
    • Updated Jest tests for style mixins
  • Release checklist
    • A changelog entry exists and is marked appropriately.
  • Designer checklist - N/A, should not have changed, although some form color variables might need to be checked in Figma

- this wasn't migrated correctly 20 months ago, `0.05` = `2%`
@cee-chen cee-chen marked this pull request as ready for review May 17, 2024 23:50
@cee-chen cee-chen requested a review from a team as a code owner May 17, 2024 23:50
@JasonStoltz JasonStoltz self-requested a review May 20, 2024 16:25
@JasonStoltz
Copy link
Member

Hey @cee-chen.

For b03db7f, I'm trying to understand what this is actually doing. What is the before and after? Does this impact all form components, and in what way?

@cee-chen
Copy link
Member Author

I actually just noticed while experimenting with autofill colors for a future PR that that change is slightly broken on dark mode, so please hold as I force push up a quick change! I'll explain a step by step of the changes made as well here in a second

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Hey @cee-chen.

I just had a couple of questions. I'm mainly trying to understand what is visually or functionally different before and after this PR.

I think it would be useful to enumerate those explicitly in the PR description, rather than trying to infer them based on the commit log. For instance:

  1. Background colors for disabled states is now a slightly different shade of gray
  2. Border colors changed from X to Y
  3. Fixed a bug in invalid/compact date picker states. Before it was X and it has been updated to Y.

I'm less concerned about the refactors and cleanup, I glanced at those but didn't fully grok all of them. For those, I trust your judgement and and more concerned about visual regressions, so I focused on QA.

@cee-chen
Copy link
Member Author

cee-chen commented May 20, 2024

I think it would be useful to enumerate those explicitly in the PR description, rather than trying to infer them based on the commit log. For instance:
Background colors for disabled states is now a slightly different shade of gray
Border colors changed from X to Y
Fixed a bug in invalid/compact date picker states. Before it was X and it has been updated to Y.

There shouldn't be any of the above kind of changes; they would have been noted in the final changelog if so. Reviewing by commit is always optional if refactors are not a primary concern, simply completing the QA steps are more than sufficient.

The border color should not have materially changed (to the visual eye) from production, although that is the most complicated set of "changes" as Elizabet already pseudo-changed it when she set up euiFormVariables a long time ago, so this was more of a change from a change. I'll dive into that per your request above in a separate thread.

@cee-chen cee-chen force-pushed the emotion/field_text branch 2 times, most recently from ff8a5a3 to 9c1f7b8 Compare May 20, 2024 18:53
Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

@cee-chen Got it. I think I misunderstood some of your commit messages. I'm good with these changes.

Comment on lines -47 to +56
borderColor: transparentize(euiTheme.border.color, 0.9),
borderDisabledColor: transparentize(euiTheme.border.color, 0.9),
borderColor: transparentize(
colorMode === 'DARK'
? euiTheme.colors.ghost
: darken(euiTheme.border.color, 4),
0.1
),
Copy link
Member Author

@cee-chen cee-chen May 20, 2024

Choose a reason for hiding this comment

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

@JasonStoltz Okay, sorry about all the force pushes! To address some of your previous unthreaded comments:

For 837a3ef I'm trying to understand what this is actually doing. What is the before and after? Does this impact all form components, and in what way?

  • borderDisabledColor was not meaningfully being used anywhere and was the exact same color as borderColor so I removed it. It's possibly an artifact of the old theme and I don't think it needs to be kept around, as it confuses code and is an extra color function that takes time to calculate at runtime.

  • transparentize(euiTheme.border.color, 0.9) was, I believe, both an incorrect transparentize level and an an opinionated change Elizabet made compared to the old Sass color function:

$euiFormBorderOpaqueColor: shadeOrTint(desaturate(adjust-hue($euiColorPrimary, 22), 22.95), 26%, 100%) !default;
$euiFormBorderColor: transparentize($euiFormBorderOpaqueColor, .9) !default;

As you can see, $euiFormBorderOpaqueColor is unnecessarily wild (desaturating our primary color?? why?) which is likely why we moved away from it and to euiTheme.border.color (much simpler).

However, transparentize 0.9 is not the right amount compared to production, which has an alpha of 0.1 compared to 0.9 - we needed to flip that to 0.1 to match production, otherwise the box-shadow overlaps the bottom linear gradient underline, like so:

0.9 0.1

Again, this likely went unnoticed for a while as euiFormVariables() was not being widely used by production form components.

Copy link
Member Author

@cee-chen cee-chen May 20, 2024

Choose a reason for hiding this comment

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

[adding to the thread]

@cee-chen Got it. I think I misunderstood some of your commit messages. I'm good with these changes.

You're totally fine, honestly this was kind of a lot to deal with conversion-wise, and I got pretty lost in the sauce of trying to figure out what was working and what wasn't + fixing existing mistakes in previous conversions. Thanks for making me slow down and write this all out in plain English!

- transparency is way not low enough right now, it needs to be `0.1` in order to not sit on top of the underline background linear gradient

- as a result we need to darken the heck out of the border color - still not a 1:1 migration as the old color's Sass calculations were semi-ludicrous, but definitely close enough

+ replace static `1px` with a token

fk
- not sure why we weren't setting it before! readonly styles will also reuse this

+ simplify `euiPlaceholderPerBrowser` to be more agnostic
- goal is to reuse these DRYly where possible
- we should use `euiFormControlStyles` instead

+ fix one EuiRange usage to just use `euiFormVariables` directly instead, the extra logic is really pretty unnecessary
- use inline styles to set the CSS variables dynamically based on props, which lets us reduce compressed overrides

- store icon size affordances in variable obj
- `.euiFormControlLayout--group` still needs to be converted separately, but I'll grab this as a separate PR
- static classNames no longer work, but thankfully we can just set a CSS variable now which inherits automatically, wee

- clean up group styles significantly as well, lots of things no longer needed after Emotion
…its classNames

- icon affordances are still somewhat repeated, but will have to wait for a future PR
…pressed inputs

+ simplify CSS to use `height: 100%` instead

+ clean up conditional Emotion styles
- fixes/removes `--inGroup` modifier className usage in docs

- not sure if this is the best approach long-term, but it's fairly straightforward for now. I'll need to re-evaluate at the end of the full migration
- more specific in name / it's clearer that it referes to 1 element of the form rather than the entire form

- better matches the other CSS var we just added
- replace with actual component
- not sure what that `!important` was doing there 🤦
- Borked this when I moved `border: none` to `euiFormControlDefaultShadow`
@cee-chen
Copy link
Member Author

Thanks for helping me slow down and catch several issues Jason! I'll go ahead and merge (a little more YOLO-y than I would normally, as this is going into a feature branch) once staging is done building.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 71c016a into elastic:emotion/forms May 20, 2024
4 checks passed
@cee-chen cee-chen deleted the emotion/field_text branch May 20, 2024 20:56
Ikuni17 pushed a commit to elastic/kibana that referenced this pull request Jun 21, 2024
`v95.0.0-backport.0` ⏩ `v95.1.0-backport.0`

This PR primarily concerns converting multiple common/building block
form control components to Emotion (text, number, and search fields).
This means that custom CSS or direct `className` usage of these form
controls **should be manually QA'd** to ensure they still look the same
before visually, with no regressions.

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

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

- Updated `EuiFormControlLayout` to automatically pass icon padding
affordance down to child `input`s
([#7799](elastic/eui#7799))

**Bug fixes**

- Fixed broken focus/invalid styling on compressed `EuiDatePickerRange`s
([#7770](elastic/eui#7770))

**CSS-in-JS conversions**

- Converted `EuiFieldText` to Emotion
([#7770](elastic/eui#7770))
- Updated the autofill colors of Chrome (and other webkit browsers) to
better match EUI's light and dark mode
([#7776](elastic/eui#7776))
- Converted `EuiFieldNumber` to Emotion
([#7802](elastic/eui#7802))
- Converted `EuiFieldSearch` to Emotion
([#7802](elastic/eui#7802))
- Converted `EuiFieldPassword` to Emotion
([#7802](elastic/eui#7802))
- Converted `EuiTextArea` to Emotion
([#7812](elastic/eui#7812))
- Converted `EuiSelect` to Emotion
([#7812](elastic/eui#7812))
- Converted `EuiSuperSelect` to Emotion
([#7812](elastic/eui#7812))

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

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

- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([#7813](elastic/eui#7813))
bhapas pushed a commit to bhapas/kibana that referenced this pull request Jun 24, 2024
`v95.0.0-backport.0` ⏩ `v95.1.0-backport.0`

This PR primarily concerns converting multiple common/building block
form control components to Emotion (text, number, and search fields).
This means that custom CSS or direct `className` usage of these form
controls **should be manually QA'd** to ensure they still look the same
before visually, with no regressions.

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

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

- Updated `EuiFormControlLayout` to automatically pass icon padding
affordance down to child `input`s
([elastic#7799](elastic/eui#7799))

**Bug fixes**

- Fixed broken focus/invalid styling on compressed `EuiDatePickerRange`s
([elastic#7770](elastic/eui#7770))

**CSS-in-JS conversions**

- Converted `EuiFieldText` to Emotion
([elastic#7770](elastic/eui#7770))
- Updated the autofill colors of Chrome (and other webkit browsers) to
better match EUI's light and dark mode
([elastic#7776](elastic/eui#7776))
- Converted `EuiFieldNumber` to Emotion
([elastic#7802](elastic/eui#7802))
- Converted `EuiFieldSearch` to Emotion
([elastic#7802](elastic/eui#7802))
- Converted `EuiFieldPassword` to Emotion
([elastic#7802](elastic/eui#7802))
- Converted `EuiTextArea` to Emotion
([elastic#7812](elastic/eui#7812))
- Converted `EuiSelect` to Emotion
([elastic#7812](elastic/eui#7812))
- Converted `EuiSuperSelect` to Emotion
([elastic#7812](elastic/eui#7812))

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

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

- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([elastic#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([elastic#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([elastic#7813](elastic/eui#7813))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants