[EuiSuperDatePicker] Convert quick select styles to Emotion #7909
Conversation
cee-chen
force-pushed
the
emotion/super-date-picker-3
branch
from
0826e0a to
4237359
Compare
| ${logicalCSS('margin-top', euiTheme.size.s)} | ||
| ${logicalCSS( | ||
| 'max-height', | ||
| mathWithUnits(euiTheme.size.m, (x) => x * 12) |
There was a problem hiding this comment.
Just wanted to highlight * 12 as an intentional change from the previous * 11 in Sass. This extra ~12px of height allows us to remove a custom CSS override in Kibana: https://github.com/elastic/kibana/blob/1a6ab7e85ad36ec17a0333568c0393b279f22fff/src/plugins/unified_search/public/query_string_input/query_bar.scss#L6-L9
| > | ||
| {(quickSelectTitle: string) => ( | ||
| <div aria-hidden className="euiFormLabel"> |
There was a problem hiding this comment.
I also wanted to highlight this as an intentional change:
| Before | After |
|---|---|
 |
 |
The Quick select title/label now uses EuiTitle and matches the other panels instead of using a custom .euiFormLabel className (which would have been unusable once form labels were converted to Emotion).
The screen reader only vs aria-hidden text is now no longer necessary as well with a slight absolute positioning CSS hack.
...i/src/components/date_picker/super_date_picker/quick_select_popover/quick_select_popover.tsx
Show resolved
Hide resolved
- not used anywhere in the EUI repo or Kibana
- much more straightforward and less custom CSS + replace className with a data-test-subj instead (used by several test selectors in Kibana)
- other styles in this file are kind of all over the place and needs more refactoring
- contains all the required styling without requiring other usages to set classNames directly - should also be flexible enough for both `div` and `fieldset/legend` usage
…ubcomponent + refactor recently used panel to use a flex column with gap instead of margins + reorder imports
+ fix incorrect fieldset/legend usage - legend has to be the first child of fieldset to be usable. Plus the label isn't that helpful anyway, so let's create our own
- fixes the label looking slightly different from the other panels + improve fieldset/legend workaround by absolutely positioning the arrows instead - a11y tweaks - remove extra verbose screen reader only text, but add `aria-describedby` to make up for it + update remaining quick select tests from Enzyme to RTL
- referencing a removed className - use data-test-subj instead
cee-chen
force-pushed
the
emotion/super-date-picker-3
branch
from
4237359 to
25bf162
Compare
💚 Build Succeeded
History
|
There was a problem hiding this comment.
I noticed a change that was actually already introduced in the first EuiSuperDatePicker Emotion conversion PR, I think it might be this change here.
Looks like the min-width changed, which results in differences for the autoRefresh variant. Was that intentional?
Screen.Recording.2024-07-26.at.09.20.52.mov
| } | ||
| `, | ||
| euiQuickSelectPanel__title: css` | ||
| float: left; /* Required for fieldset/legend elements */ |
There was a problem hiding this comment.
This float is now added to span as well as legend and I see this causes some differences in spacing:
Screen.Recording.2024-07-26.at.09.06.51.mov
There was a problem hiding this comment.
The new version is actually more correct - we want the <span> to render more like a display block, which float automatically does. It looks like production was missing that
packages/eui/src/components/date_picker/super_date_picker/quick_select_popover/quick_select.tsx
Show resolved
Hide resolved
packages/eui/src/components/date_picker/super_date_picker/quick_select_popover/quick_select.tsx
Show resolved
Hide resolved
I'm not able to repro your screencap in either local dev or on the staging PR - what browser are you on, and what are the repro steps for it? |
@cee-chen I opened the docs for auto-refresh and clicked the EuiSuperDatePicker input to open the popover. I see the difference for Chrome and Edge (inputs next to each other on branch vs below each other on production). In Safari and Firefox they are the same (except for expected width changes). I'm realizing now, that it might actually have been unexpected in Chrome/Edge before and it's fixed now 😄 So please disregard then! |
|
Oooo gotcha! They definitely should not be below each other, so huzzah for a bug unintentionally fixed! |
`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 <[email protected]>
Summary
Part 3 of 3 (see #7904, #7908)
Converts EuiSuperDatePicker's quick select popover/button styles to Emotion. I strongly recommend following along by commit as I ended up creating a new internal
EuiQuickSelectPanelsubcomponent for styling, and also noticing a bunch of missing i18n in the refresh interval component. I also strongly recommend hiding whitespace changes while viewing diffs as there are several sets of changes that are just indentation.QA
General checklist
- [ ] Added or updated jest and cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)