-
Notifications
You must be signed in to change notification settings - Fork 841
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
Conversation
- this wasn't migrated correctly 20 months ago, `0.05` = `2%`
f3efeb2
to
5a186a5
Compare
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 |
There was a problem hiding this 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:
- 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.
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.
6edee4c
to
973f499
Compare
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 |
ff8a5a3
to
9c1f7b8
Compare
There was a problem hiding this 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.
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 | ||
), |
There was a problem hiding this comment.
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 asborderColor
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:
eui/packages/eui/src/global_styling/variables/_form.scss
Lines 32 to 33 in 5c40315
$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.
There was a problem hiding this comment.
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`
9c1f7b8
to
5f59c90
Compare
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. |
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
`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))
`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))
Summary
This PR converts the
EuiFieldText
component to Emotion + all the form mixins required for its styles, and updates several downstream components that dogfoodEuiFieldText
(EuiColorPicker
andEuiDatePicker
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 equivalentseuiFormControlSize()
JS mixin in favor of a neweuiFormControlStyles()
, which outputs an object of multiple modifiers/states for reuse--euiFormControlLeftIconsCount
and--euiFormControlRightIconsCount
respectively) set dynamically via inline styles, rather than static classNamesEuiFieldText
and reusing its classNames for styling had to be updated. For example,EuiDatePicker
was updated to dogfood theEuiFieldText
component directly instead of an<input type="text" />
QA
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes