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

Fix time selection in EuiSuperDatePicker #1704

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Mar 8, 2019

Summary

Fixes #1596
Fixes #1711

The time selector had a componentDidUpdate that would always scroll the pre-selected time into view, which was implemented for keyboard up/down arrows to properly navigate the list. I've updated that function's logic to:

  • scroll to the selected time, if it has changed
  • scroll to the preselected time if instructed to do so, tied only to the up/down arrow key events

I also simplified the scrolling to the selected/preselected time in componentDidMount, now unified across the react-datepicker docs and our super date picker where we changes the scrollable element.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@chandlerprall
Copy link
Contributor Author

jenkins test this 🎵flaky test 🎶

@thompsongl
Copy link
Contributor

🤔

12:22:25 Summary of all failing tests
12:22:25 FAIL src/components/toast/global_toast_list.test.js

Are these just flaky or do we have problems on master?

@thompsongl
Copy link
Contributor

lol

@thompsongl
Copy link
Contributor

One change from the previous behavior: tabbing into the time list always scrolls the list to the top. Couldn't tell from the summary if this change is intentional or not

Kapture 2019-03-08 at 13 22 47

@chandlerprall
Copy link
Contributor Author

@snide the issue Greg mentions above is because the overflowed div is getting scrolled back "into view"; any ideas?

scroll issue on focus

@snide
Copy link
Contributor

snide commented Mar 11, 2019

@chandlerprall I'll take a look, but it's definitely a better tradeoff.

@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2019

If the overflow-y scroll is moved from the .react-datepicker__time to the .react-datepicker__time-list it doesn't auto-scroll when tabbing.

However, the calculated height on .react-datepicker__time-list doesn't expand the full height of the container.

Screen Shot 2019-03-11 at 14 39 25 PM

@chandlerprall
Copy link
Contributor Author

@thompsongl given Dave's comment above on the tradeoff, mind giving this a full review?

@snide
Copy link
Contributor

snide commented Mar 11, 2019

@chandlerprall @cchaos I should have a fix incoming. But you can review everything else.

@snide
Copy link
Contributor

snide commented Mar 11, 2019

Committed a fix in. Nothing a few flex grows can't fix! Thanks for the quick read @cchaos

@snide
Copy link
Contributor

snide commented Mar 11, 2019

There's a non-breaking, but annoying IE11 issue with my change. Fixing that now

@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2019

@chandlerprall Is there no way to retain auto-scrolling to the selected time on open?

@chandlerprall
Copy link
Contributor Author

@cchaos derp I hadn't pushed all my changes up, and forgot about them trying to solve that scroll-on-tab-focus issue

@snide
Copy link
Contributor

snide commented Mar 11, 2019

Added a fix for #1711 while i was in there

image

CHANGELOG.md Outdated

**Bug fixes**

- Added button to `EuiSuperDatePicker`'s “Now” tab to trigger the "now" time selection ([#1620](https://github.com/elastic/eui/pull/1620))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this keep moving? It's technically not a bug fix....

@snide
Copy link
Contributor

snide commented Mar 11, 2019

OK. I'm out of this PR. IE works and shouldn't have weird stretches anymore.

@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2019

I think a bad merge/rebase happened here.

…erprall/eui into bug/1596-datepicker-time-selector
@thompsongl
Copy link
Contributor

derp I hadn't pushed all my changes up, and forgot about them trying to solve that scroll-on-tab-focus issue

Still missing this changeset, I think?

@chandlerprall
Copy link
Contributor Author

Logic conflicted with Dave's changes, I've updated and re-pushed. @cchaos @thompsongl I believe everything's ready for testing again.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Changes LGTM.
Manual test results in expected scroll behavior

Still need changelog

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Fan freakin fantastic! 🥇

@chandlerprall chandlerprall merged commit cc4f3f9 into elastic:master Mar 11, 2019
@chandlerprall chandlerprall deleted the bug/1596-datepicker-time-selector branch March 11, 2019 22:34
Shigawire pushed a commit to Shigawire/eui that referenced this pull request May 10, 2019
* Update react-datepicker time selector to not _always_ scroll to preSelection time

* Update react-datepicker time selection scroll-into-view onMount logic

* revert props default changes I made for testing

* fix scroll issue

* fix ie issue

* A few more dark mode fixes (elastic#1700)

* 9.2.0

* Updated documentation.

* Make EuiPopover's repositionOnScroll prop optional in TS (elastic#1705)

* Make EuiPopover's repositionOnScroll prop optional in TS

* changelog

* fix range coloring

* Fix scrollTop target

* changelog
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.

4 participants