[Emotion] Convert EuiDatePicker #7937
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `.react-datepicker__screenReaderOnly` - doesn't exist in DOM, plus we already have EuiScreenReaderOnly - `.react-datepicker__header` - isn't doing anything, children are absolutely positioned - `.react-datepicker-time__header` - is being `display: none`'d elsewhere so why are we truncating it - `.react-datepicker__month` - individual days are already centered, there's no other styling so border-radius isn't necessary
+ remove Sass variable
`.react-datepicker__header--time` only has screen reader text in it and does not need to be `display: none`'d
- allows us to use `className` instead of HOCs for the underlying components + delete variable with only one usage
…lements to Emotion + handle time select responsive stacking
+ add new stories for testing + quick VRT demos of converted CSS + fix incorrect horizontal cut-off of background on prod - use `margin auto` instead of `width: 100%` + tweak inline datepicker range to not change time select padding, and adjust container query accordingly
+ write a bunch of stories for VRT / QA to test things got converted propertly
+ remove unnecessary overflow/flex workaround by changing react-datepicker code to sort earliest first in the array
- it's only being used in one place, so I guess just inline it? No idea where the width even came from
cee-chen
force-pushed
the
emotion/react-date-picker
branch
from
0264588
to
cd5a188
Compare
cee-chen
commented
Aug 1, 2024
packages/eui/.loki/reference/chrome_mobile_Forms_EuiDatePickerRange_Inline.png
Outdated
Show resolved
Hide resolved
mgadewoll
reviewed
Aug 1, 2024
packages/eui/src/components/date_picker/react-datepicker/src/calendar.js
Show resolved
Hide resolved
packages/eui/.loki/reference/chrome_mobile_Forms_EuiDatePickerRange_Inline.png
Outdated
Show resolved
Hide resolved
… production builds
- the `.react-datepicker__header--time` was what was messing with the flex behavior - applying absolute positioning to it fixes the issue via EuiScreenReaderOnly + remove unnecessary inline `height` behavior - flex handles everything for us + bonus - decrease gapSize on mobile, it's a little too large now (opinion)
…on on mount - via `scrollIntoView` rather than a manual calc - the `window.scroll` workaround feels worth the tradeoff + remove now-unnecessary refs
💚 Build Succeeded
History
|
mgadewoll
approved these changes
Aug 2, 2024
|
Thanks as always for the amazing review Lene!! 🔥 |
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Aug 5, 2024
`v95.5.0` ⏩ `v95.6.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.6.0`](https://github.com/elastic/eui/releases/v95.6.0) - Updated `EuiIcon` with a new `crossInCircle` glyph ([#7924](elastic/eui#7924)) **Bug fixes** - Fixed `EuiEmptyPrompt` to correctly collapse and expand responsively when used with custom breakpoints larger than the default `xl` breakpoint ([#7935](elastic/eui#7935)) **Accessibility** - Improved the experience of `EuiModal` by ensuring nested `EuiPopover` closes on `Escape` keypress instead of the modal ([#7939](elastic/eui#7939)) **CSS-in-JS conversions** - Converted `EuiDatePicker` to Emotion ([#7937](elastic/eui#7937)) - Removed `$euiDatePickerCalendarWidth` - Removed `$euiDatePickerPadding` - Removed `$euiDatePickerGap` - Removed `$euiDatePickerCalendarColumns` - Removed `$euiDatePickerButtonSize` - Removed `$euiDatePickerMinControlWidth` - Removed `$euiDatePickerMaxControlWidth` - Removed `@mixin datePickerCaret` - Removed `@mixin datePickerArrow`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR converts
EuiDatePickerand the underlyingreact-datepickertheme/custom styles to Emotion. For the latter, it heavily relies on nested 3rd party selectors - I consider modifyingreact-datepickerdirectly to use Emotion to be out of scope / unnecessary for now.This is a fairly hefty PR and contains a ton of cleanup (there was a lot of unused CSS), so I strongly recommend following along by commit and relying primarily on QA.
QA
The below components should generally look the same as production:
General checklist
- [ ] Added or updated jest and cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)Emotion checklist
General
className(s)read as expected in snapshots and browsersUnit tests
shouldRenderCustomStyles()test was added and passes with parent component and any nestedchildProps(e.g.tooltipProps)Sass/Emotion conversion process
src/components/index.scsssrc/amsterdam/overrides/{component}.scssfiles (styles within should have been converted to the baseline Emotion styles)$euiSizetoeuiTheme.size.base)[ ] Added an@warndeprecation message within theglobal_styling/mixins/{component}.scssfile[ ] Removed or converted component-specific Sass vars/mixins to exported JS versions[ ] Ranyarn compile-scssto update var/mixin JSON files[ ] Simplifiedcalc()tomathWithUnitsif possible (if mixing different unit types, this may not be possible)CSS tech debt
euiCanAnimategapproperty to add margin between items if using flex-inlineand-blocklogical properties (check inline styles as well as CSS)DOM Cleanup
euiComponent,euiComponent__child)euiDatePicker--usagesKibana due diligence
{euiComponent}-(case sensitive) to check for usage of modifier classes[ ] If usage exists, consider converting to adataattribute so that consumers still have something to hook into**/target, **/*.snap, **/*.storyshotfor less noise) foreui{Component}(case sensitive) to find:euiBadgeclass on a div instead of simply using theEuiBadgecomponent)Extras/nice-to-have
[ ] Reduced specificity where possible (usually by reducing nesting and class name chaining)[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about[ ] Documentation pass[ ] Check for issues in the backlog that could be a quick fix for that component