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 EuiTextArea, EuiSelect, and EuiSuperSelect #7812

Merged
merged 16 commits into from
Jun 6, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jun 4, 2024

Summary

Note

This PR merges into a feature branch.

I strongly recommend following along by commit, especially for the select components which required more complex cleanup or changes.

QA

  • EuiTextArea - Docs and Storybook look the same as before with no obvious regressions
  • EuiSelect - Docs and Storybook look the same as before with no obvious regressions
  • EuiSuperSelect - Docs and Storybook look the same as before with no obvious regressions

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
  • 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 - N/A

cee-chen added 12 commits June 4, 2024 14:49
+ remove className map

+ simplify `definedRows` variable
- no impact on modern/latest browsers of Edge or Firefox
- memoization
- code order
- reduce inline let's
- trying to avoid having to wrap a class component with a generic type in a theme HOC, so opting for static className util instead
+ create new subcomponent util to make it easier to use style hooks

+ move location of Option type outside of control (makes slightly more sense here, file-wise)
- our more granular/explicit architecture (vs an all in one mixin) pays off!! 🎉

- we now also get to remove a `ts-ignore` + invalid `readonly` attribute (doesn't apply to buttons)

- also remove extra className modifier which isn't great BEM (no Kibana usages)
@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 4, 2024

@mgadewoll I probably should have asked this before now, but should I be updating VRT screenshots at the same time as as I open these PRs? 😅

@mgadewoll
Copy link
Contributor

@mgadewoll I probably should have asked this before now, but should I be updating VRT screenshots at the same time as as I open these PRs? 😅

Will there be another PR to merge the form branch into main? If so, I think it's fine to do the VRT there. The separate PRs might cause changes in the VRT images between separate PRs so doing it once all is done might be less cumbersome 🙂

@cee-chen cee-chen marked this pull request as ready for review June 5, 2024 15:19
@cee-chen cee-chen requested a review from a team as a code owner June 5, 2024 15:19
@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 5, 2024

Will there be another PR to merge the form branch into main? If so, I think it's fine to do the VRT there. The separate PRs might cause changes in the VRT images between separate PRs so doing it once all is done might be less cumbersome 🙂

I was thinking more if it made it easier to see the visual difference in CSS changes! I'm also fine doing one final VRT pass in the final feature branch merge.

@mgadewoll
Copy link
Contributor

Will there be another PR to merge the form branch into main? If so, I think it's fine to do the VRT there. The separate PRs might cause changes in the VRT images between separate PRs so doing it once all is done might be less cumbersome 🙂

I was thinking more if it made it easier to see the visual difference in CSS changes! I'm also fine doing one final VRT pass in the final feature branch merge.

I don't mind either way as long as we run it at some point 🙂 In any case I think we need to always make sure to run VRT after additional changes (while we don't have a pipeline step yet)

@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 5, 2024

Comparing the VRT screenshots for this PR actually just helped me catch a bug I missed so I'm definitely doing them from now on!!

@@ -1,9 +0,0 @@
.euiSuperSelect__item {
@include euiFontSizeS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's intended, but I think this mixin is missing in the emotion version?
I'm seeing differences due to that in the option content height (line-height specifically is different now)

super_select_emotion_conversion.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot, I did totally miss this! 🤦 Thank you so much for catching it!

Copy link
Contributor Author

@cee-chen cee-chen Jun 6, 2024

Choose a reason for hiding this comment

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

Actually, it looks like the new Emotion CSS-in-JS euiFontSize(euiThemeContext, 's') changes the line height to be smaller anyway (Caroline intentionally changed this as part of Emotion work) so I think the reduced height would have been an intentional change in any case. (and IMO, it looks pretty good!)

I'm going to go ahead and explicitly add it in any case because it's better to be clear than not clear 🤷 c2f14e3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just mentioning for my own memory - @include euiInteractiveStates didn't appear to do anything / was already handled by EuiContextMenuItem, and can be safely removed

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for the additional changes and contextual info! All looks good to me! 🚀

@mgadewoll
Copy link
Contributor

mgadewoll commented Jun 6, 2024

Ah wait - I think you might need to rerun VRT? 😄

update: I correct myself, the visual changes are not visible in the VRT yet I think 😅

@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 6, 2024

I'm not familiar enough with VRT to know if there's a way to get it into certain states before a snapshot is taken. Clicking the dropdown to toggle options would be great for example! There are multiple stories that would benefit from this!

@cee-chen cee-chen merged commit 5e06197 into elastic:emotion/forms Jun 6, 2024
4 checks passed
@cee-chen cee-chen deleted the emotion/textarea-selects branch June 6, 2024 21:10
@mgadewoll
Copy link
Contributor

I'm not familiar enough with VRT to know if there's a way to get it into certain states before a snapshot is taken. Clicking the dropdown to toggle options would be great for example! There are multiple stories that would benefit from this!

That's also fine for now. There are a lot of components that don't have that enabled yet 😅
We can do simple interactions already with the loki interactions. We just currently have some issue that sometimes the loki selector for portals does not work. Here is an example (VRT image) for EuiComboBox that currently takes a screenshot of the entire body to side-step that issue for now.

@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 6, 2024

Oooh gotcha, thanks for clarifying!

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