[EuiSuperDatePicker] Convert form control styles to Emotion by cee-chen · Pull Request #7904 · elastic/eui

cee-chen · 2024-07-22T21:21:10Z

Summary

This PR converts EuiSuperDatePicker's form control UI to Emotion. It does not convert any of EuiSuperDatePicker's popover styles to Emotion (for ease of review - plus to help separate some extra groundwork with EuiDatePicker's variables).

QA

https://eui.elastic.co/pr_7904/#/templates/super-date-picker should look the same as production with the following permutations:

States
- When focused (click the Last 30 minutes text then click between ~30 minutes ago and now)
- When invalid (click the Last 30 minutes text, then ~30 minutes ago, then change to minutes from now)
- When in "needs update" mode (click the Last 30 minutes text, then ~30 minutes ago, then change to seconds ago from now)
- When disabled (use Storybook for this and manually type in true to the isDisabled control)
Widths
- When in restricted width mode (600px wide)
- When in full width mode
- When in auto width mode
- When compressed

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
- [ ] Added or updated jest and cypress tests
- Updated visual regression tests
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

Emotion checklist

General

Output CSS matches the previous CSS (works as expected in all browsers)
Rendered className(s) read as expected in snapshots and browsers
~[ ] Checked component playground ~

Unit tests

shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
Convert Enzyme tests to RTL

Sass/Emotion conversion process

Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
Removed or converted component-specific Sass vars/mixins to exported JS versions
Listed var/mixin removals in changelog
~~[ ] Ran yarn compile-scss to update var/mixin JSON files~~
~~[ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~
~~[ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~
~~[ ] Removed component from src/components/index.scss~~ - Not yet
~~[ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)~~

CSS tech debt

Wrapped all animations or transitions in euiCanAnimate
Used gap property to add margin between items if using flex
Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)

DOM Cleanup

Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)
SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Kibana due diligence

Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
Any test/query selectors that will need to be updated - None
Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted - None
Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component) - None
Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes - None for EuiSuperDatePicker, 2 for EuiDatePopoverButton
- ~~[ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into~~

Extras/nice-to-have

Reduced specificity where possible (usually by reducing nesting and class name chaining)
~~[ ] Documentation pass:~~
~~[ ] Check for issues in the backlog that could be a quick fix for that component~~
~~[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about~~

- some shenanigans rquired because of existing render fn

+ delete width Sass variables

- `noUpdateButton` requires a few per-width-type tweaks to get this working as before in prod, due to changed CSS delcaration order - `isAutoRefreshOnly` is not correctly responding to width prop on prod, this fixes that

- flatten CSS where possible (in the case of `.euiFormControlLayout__childrenWrapper`'s border-radius it is sadly not possible) - un-DRY prettyFormat CSS for now, DRY out with popover buttons later - `!important` on `.euiSuperDatePicker__rangeInput` required to offset the CSS added by `fullWidth: true` on the form control layout

+ simplify `.euiDatePopoverButton` to inherit from the form wrapper instead of reusing the static variable

- use flex for compressed height instead of static form variables - keep modifier classes for Kibana usage

- [opinionated cleanup/change] set background colors on the layout wrapper div instead of the button where possible - reuse the CSS variable and form Emotion mixins where possible as well

cee-chen · 2024-07-22T23:03:50Z

packages/eui/src/components/date_picker/super_date_picker/super_date_picker.test.tsx

-    component.setProps({ isPaused: true, refreshInterval: 0 });
-    jest.advanceTimersByTime(10);
-    await instanceRefresh.asyncInterval!.__pendingFn;
+    await jest.runAllTicks();


@mgadewoll Your Jest upgrade PR allowed me to use this new handy util!! 🎉

+ fix test describe name

elasticmachine · 2024-07-23T01:18:06Z

💚 Build Succeeded

Buildkite Build
Commit: 1583a79

History

💔 Build #2365 failed 8146a82
💚 Build #2363 succeeded b61fe26
💚 Build #2362 succeeded 5bd947d

mgadewoll

I left two small comments for clarification purposes on changes but apart from that the changes look good to me!

mgadewoll · 2024-07-23T12:45:55Z

packages/eui/src/components/date_picker/super_date_picker/super_date_picker.styles.ts

+
+    widths: {
+      restricted: css`
+        ${logicalCSS('width', restrictedWidth)}


Just for clarification: I noticed there is a tiny change in width here now, that I assume is expected? I see that previously the EuiSuperDatePicker had a restricted width of 606px and now 600px which seems more sensible.

Screen.Recording.2024-07-23.at.14.22.37.mov

Yeah I think the math was off with the previous Sass calculations (possibly due to flex gap?). Agreed that 600px seems more correct than 606px!

mgadewoll · 2024-07-23T12:50:39Z

packages/eui/src/components/date_picker/super_date_picker/super_date_picker.styles.ts

+
+      &:disabled {
+        background-color: ${forms.backgroundDisabledColor};
+        color: ${forms.controlDisabledColor};


I noticed this color changed noticeably and reducing the color contrast quite a lot. Is this expected?

Screen.Recording.2024-07-23.at.14.34.03.mov

before

after

IMO it's more correct as it matches our existing form controls/fields:

It's also worth noting that there are no WCAG color contrast requirements for disabled elements/buttons: https://sean-elliott.medium.com/a11y-tips-disabled-buttons-and-colour-contrast-f8824d5e9610

Agreed, that overall this change makes sense to align with other form controls. I was just wondering if we consider the contrast, but if that would be the case it's anyway a question on a broader level for forms I guess.

Yes, and this way/with this change, if we change contrast levels for all form components, this component will also update automatically :)

mgadewoll · 2024-07-23T13:02:39Z

packages/eui/src/components/date_picker/super_date_picker/super_date_picker.test.tsx

-    expect(componentUpdatedRefresh.prop('isPaused')).toBe(false);
+    const refreshButton = container.querySelector('.euiAutoRefreshButton');
+
+    expect(refreshButton).toBeInTheDocument();


Much cleanup, much nice 🧹

cee-chen · 2024-07-23T16:21:42Z

Going to go ahead and merge this PR to move ahead on more EuiSuperDatePicker emotion work. If you have any other change requests that I didn't fully address I can grab it in a follow-up PR!

`v95.4.0` ⏩ `v95.5.0` _[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.5.0`](https://github.com/elastic/eui/releases/v95.5.0) - Added `minusInSquare` and `plusInSquare` glyphs to `EuiIcon`. ([#7875](elastic/eui#7875)) **Bug fixes** - Fixed `EuiSuperDatePicker` not correctly passing `refreshMinInterval` from the quick select popover ([#7905](elastic/eui#7905)) **CSS-in-JS conversions** - Converted `EuiSuperDatePicker`'s form control to Emotion; ([#7904](elastic/eui#7904)) - Removed `$euiSuperDatePickerWidth` - Removed `$euiSuperDatePickerButtonWidth` - Removed `$euiSuperDatePickerNeedsUpdatingBackgroundColor` - Removed `$euiSuperDatePickerNeedsUpdatingTextColor` - Removed `@euiSuperDatePickerText` mixin - Converted `EuiSuperDatePicker`'s date popover content to Emotion ([#7908](elastic/eui#7908)) - Converted `EuiSuperDatePicker`'s quick select to Emotion ([#7909](elastic/eui#7909)) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

cee-chen added 8 commits July 22, 2024 09:41

Initial Emotion setup

6cd8fce

- some shenanigans rquired because of existing render fn

Convert date picker widths and width modifiers to Emotion

3c9a6c5

+ delete width Sass variables

Fix/tweak various width permutations

e6c92d0

- `noUpdateButton` requires a few per-width-type tweaks to get this working as before in prod, due to changed CSS delcaration order - `isAutoRefreshOnly` is not correctly responding to width prop on prod, this fixes that

Convert needsUpdating state

220a448

+ simplify `.euiDatePopoverButton` to inherit from the form wrapper instead of reusing the static variable

DRY out/convert euiSuperDatePickerText mixin + EuiDatePopoverButton

c4c4d49

- use flex for compressed height instead of static form variables - keep modifier classes for Kibana usage

Convert remaining form linear gradient/underline styles

09c897e

- [opinionated cleanup/change] set background colors on the layout wrapper div instead of the button where possible - reuse the CSS variable and form Emotion mixins where possible as well

Delete Sass mixin/variables files

811ae87

cee-chen added the emotion label Jul 22, 2024

cee-chen added 2 commits July 22, 2024 14:27

changelog

5bd947d

Update visual regression snapshots

b61fe26

cee-chen force-pushed the emotion/super-date-picker-1 branch from 426d8cb to 8146a82 Compare July 22, 2024 23:01

cee-chen marked this pull request as ready for review July 22, 2024 23:03

cee-chen requested a review from a team as a code owner July 22, 2024 23:03

cee-chen commented Jul 22, 2024

View reviewed changes

cee-chen force-pushed the emotion/super-date-picker-1 branch from 8146a82 to 3abb05a Compare July 23, 2024 00:50

[tech debt] Convert Enzyme tests to RTL

1583a79

+ fix test describe name

cee-chen force-pushed the emotion/super-date-picker-1 branch from 3abb05a to 1583a79 Compare July 23, 2024 00:50

mgadewoll approved these changes Jul 23, 2024

View reviewed changes

cee-chen merged commit 4ef461c into elastic:main Jul 23, 2024

cee-chen deleted the emotion/super-date-picker-1 branch July 23, 2024 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[EuiSuperDatePicker] Convert form control styles to Emotion#7904

[EuiSuperDatePicker] Convert form control styles to Emotion#7904
cee-chen merged 11 commits intoelastic:mainfrom
cee-chen:emotion/super-date-picker-1

cee-chen commented Jul 22, 2024 •

edited by mgadewoll

Loading

Uh oh!

cee-chen Jul 22, 2024 •

edited

Loading

Uh oh!

elasticmachine commented Jul 23, 2024

Uh oh!

mgadewoll left a comment

Uh oh!

mgadewoll Jul 23, 2024 •

edited

Loading

Uh oh!

cee-chen Jul 23, 2024

Uh oh!

mgadewoll Jul 23, 2024

Uh oh!

cee-chen Jul 23, 2024

Uh oh!

mgadewoll Jul 23, 2024

Uh oh!

cee-chen Jul 23, 2024

Uh oh!

mgadewoll Jul 23, 2024

Uh oh!

cee-chen commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

cee-chen commented Jul 22, 2024 • edited by mgadewoll Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

cee-chen Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 23, 2024

💚 Build Succeeded

History

Uh oh!

mgadewoll left a comment

Choose a reason for hiding this comment

Uh oh!

mgadewoll Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

mgadewoll Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

mgadewoll Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

mgadewoll Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cee-chen commented Jul 22, 2024 •

edited by mgadewoll

Loading

cee-chen Jul 22, 2024 •

edited

Loading

mgadewoll Jul 23, 2024 •

edited

Loading