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

[EuiDatePickerRange] Add inline display behavior; [EuiDatePicker] Improve inline display behavior #6795

Merged
merged 14 commits into from
May 25, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented May 23, 2023

Summary

⚠️ This PR has several cleanup and setup tasks, so I recommend following along by commit.
It's also our first-ever use of CSS container queries in EUI! 🎉

The ML team was previously using EuiDatePickerRange and inline together, which was an undocumented usage of the range picker and was only sorta working by accident (note the buggy shadow+border behavior):

#6705 broke this accidental behavior by enforcing all the height/width styles a delimited form control normally enforces:

The solution to this is to make the inline prop an actual first class citizen for EuiDatePickerRange. In addition to correctly displaying the base inline state, responsive behavior needed to be implemented, and there were numerous issues and untested permutations of inline and, e.g. disabled/readOnly/isInvalid that needed more thoughtful handling.

EuiDatePicker

screencap

EuiDateRangePicker

screencap2

QA

  • Go to https://eui.elastic.co/pr_6795/#/forms/date-picker#date-picker-inline
  • Confirm that disabled and readOnly states for both the single and range picker display as visually expected, and cannot be clicked on or tabbed to
  • Confirm that loading and isInvalid icons appear / set a bottom line
  • Disable the show time select switch. Resize your screen and confirm the range picker collapses to a vertical display before any content is cut off
  • Enable the show time select switch. Resize your screen and confirm the range picker collapses to a vertical display before any content is cut off

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added documentation
  • Added or updated jest and cypress tests
  • Checked for accessibility including keyboard-only and screenreader modes
    • Our date picker needs more thorough SR testing in general, so I skipped SR testing for now
  • A changelog entry exists and is marked appropriately

- [ ] Checked Code Sandbox works for any docs examples
- [ ] Checked for breaking changes and labeled appropriately - there are className and DOM changes that we'll likely have to update several Kibana snapshots for, but I don't consider that to be breaking
- [ ] Updated the Figma library counterpart - not seeing an inline prop demo in Figma

@cee-chen cee-chen requested a review from tkajtoch May 23, 2023 04:36
@cee-chen
Copy link
Contributor Author

jenkins test this

cee-chen added 10 commits May 22, 2023 21:52
- `.react-datepicker-popper` is no longer a class being set anywhere

- shadow styles don't use a border
- fix `disabled` and `readOnly` styling & behavior - should match inputs and no longer be interactable

- render form control icons below the datepicker

- better docs

- write tests (e2e for now, should likely be storybook)
+ remove unnecessary height style - appears to already work as-is thanks to `.euiFormControlLayoutDelimited`
- helps separate form control and date picker range concerns
… toggle

- due to new `span`

`isLoading` needs to be specified via typescript + pulled out of `...rest`

tests
- reorganize w/ `describe`
- add missing props tests

docs
- convert to TSX
- fix bad `isInvalid` use from min test
- remove non-working `isInvalid` from `range.tsx` - the display toggle is overriding that prop
- use separate styles fn, since display is so different and has many extra conditions - requires a lot of resets and layout fixes

- update docs examples with toggles & snippet

- add unit test - but most permutations should likely be storybooks
…nd `prepend/append` display

- they don't make sense to render, and the single datepicker doesn't render them either
- to get `@container` query support, which I'll want to use for responsive `inline` behavior
…siveness

(sadly, jsdom errors on this, so we have to add another console error exclusion)
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6795/

@cee-chen cee-chen force-pushed the daterange-inline branch from e096f99 to df16cc7 Compare May 23, 2023 04:59
@cee-chen
Copy link
Contributor Author

jenkins test this

@elastic elastic deleted a comment from kibanamachine May 23, 2023
@elastic elastic deleted a comment from kibanamachine May 23, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6795/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6795/

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

This is a huge improvement! 🎉 Code looks great and the changes work as expected on all browsers. Visually I noticed a couple minor things:

  • when isInvalid = true, the bottom bar is spaced 1px off the date picker's edge due to euiFormControlLayoutDelimited having padding: 1px
    Screenshot 2023-05-25 at 11 49 38
  • EuiDatePickerRange layout wraps content at 700px container width but some of the endDateControl content becomes obstructed below 740px when showTimeSelect = true.
    Screenshot 2023-05-25 at 11 31 48

@cee-chen
Copy link
Contributor Author

Thanks for the great catches Tomasz! I'll fix the dependency conflicts and address the two visual items in your screenshot here shortly

@cee-chen cee-chen requested a review from tkajtoch May 25, 2023 16:11
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6795/

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@cee-chen cee-chen merged commit 43c9bc5 into elastic:main May 25, 2023
@cee-chen cee-chen deleted the daterange-inline branch May 25, 2023 20:04
@cee-chen cee-chen self-assigned this May 26, 2023
cee-chen added a commit to elastic/kibana that referenced this pull request Jun 5, 2023
## Summary

`eui@81.0.0` ⏩ `eui@81.2.0`

- Most changes to source code in this PR are CSS cleanups/deprecations
in `EuiSuperDatePicker`/`EuiDatePickerRange`
- One team (ML) had a `inline` specific usage of `EuiDatePickerRange`
that they reached out to us about via Slack, and that we have fixed in
this PR.
- All other usages of date picker components should have remained
working as-is with no changes, but please ping us if you see otherwise!

___

## [`81.2.0`](https://github.com/elastic/eui/tree/v81.2.0)

- Updated `EuiSuperDatePicker` to accept an object configuration for
`isDisabled` ([#6821](elastic/eui#6821))

**Bug fixes**

- Fixed broken `EuiSuperDatePicker` styles
([#6821](elastic/eui#6821))

## [`81.1.0`](https://github.com/elastic/eui/tree/v81.1.0)

- Added `EuiInlineEditText` and `EuiInlineEditTitle` components
([#6757](elastic/eui#6757))
- Updated `EuiDatePickerRange` to support `inline` display
([#6795](elastic/eui#6795))
- Added an `onError` callback prop to `EuiErrorBoundary`
([#6810](elastic/eui#6810))
- Updated `EuiDataGrid` to only render screen reader text announcing
cell position if the cell is currently focused. This should improve the
ability to copy and paste multiple cells without SR text.
([#6817](elastic/eui#6817))

**Bug fixes**

- Fixed `EuiDatePicker`'s `inline` display to correctly render and
prevent user interaction when `disabled` or `readOnly`
([#6795](elastic/eui#6795))
- Fixed `EuiDatePicker`'s `inline` display to correctly render
`isInvalid` and `isLoading` icons
([#6795](elastic/eui#6795))

**CSS-in-JS conversions**

- Converted `EuiDatePickerRange` to Emotion
([#6795](elastic/eui#6795))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants