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

React datepicker better responsive handling when inline #1820

Merged
merged 10 commits into from
Jul 2, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Apr 10, 2019

Summary

Fixes #1809

Also tried to make the overall EuiSuperDatePicker a little smaller so that it fits in smaller resolutions (it now matches the size of the calendar itself).

image

image

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

@snide snide requested a review from cchaos April 10, 2019 22:34
@snide
Copy link
Contributor Author

snide commented Apr 10, 2019

NM on those comments, forgot these were in the same folder, so it was easy. Cleaned them up.

@snide snide added the bug label Apr 10, 2019
@snide
Copy link
Contributor Author

snide commented Apr 16, 2019

@cchaos This one's been sitting for awhile. Mind giving it a quick review?

@cchaos
Copy link
Contributor

cchaos commented Apr 17, 2019

I think we need to come up with a better solution for the mobile calendar picker. I don't think we can just hide the time selects. The whole Time Selection section becomes moot because none of the inputs show a time list.

@snide
Copy link
Contributor Author

snide commented Apr 17, 2019

@cchaos I think ultimately for me the biggest thing is that the calendar is useful in a mobile situation. The time selector is nice to have when we have the space, but honestly is going to be very, very hard to scroll decently in a mobile application given the default number of results (48?). Given that, using the pure text input works perfectly fine here and will likely be faster for most than fiddling with a skinny scroll selector.

I think if we wanted to get deeper into solving the issue, we'd likely want to replace these controls with actual input type="date" fields when in mobile, which will be pretty consistently rendered and a muuuuch better experience. I think that's a fundamental rework of how we use these things though (primarily in SDP) and would need a lot more thinking and refactoring.

Given all that. I look at this as simply a quick hotfix solution we can add today to avoid breaks (as your issue points out, it is actually broken right now, and the SDP is too wide) and make the current datepicker and super datepicker work today without too much fuss.

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.

Can we change the first example to now show the time in the input? I keep thinking it's an error that I don't see a time select and you can't type in the input.

I actually think turning the time display off should be default since showing the time select is off by default.

The only thing more related to this PR is it looks like the calendar is not completely centered in the panel:

@cchaos
Copy link
Contributor

cchaos commented Jun 6, 2019

We should try to get this in. @snide Do you want me to finish up those last few items or will you have time to?

@snide
Copy link
Contributor Author

snide commented Jun 6, 2019

@cchaos go for it.

snide and others added 6 commits June 14, 2019 10:22
- Right arrow position
- Selected & in-range style
- Default `dateFormat` doesn’t include time since `showTimeFormat` is false by default
const horizontalRuleSnippet = `<EuiHorizontalRule size="quarter" />`;
const horizontalRuleMarginSnippet = `<EuiHorizontalRule margin="xs" />`;
const horizontalRuleSnippet = '<EuiHorizontalRule size="quarter" />';
const horizontalRuleMarginSnippet = '<EuiHorizontalRule margin="xs" />';
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter forced me to change these

@@ -217,7 +217,7 @@
&--next {
@include datePicker__arrow;
// Pixel value because of some padding on the icon
right: 10px;
right: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Aligned this value with the one for previous:

before:

@@ -635,7 +635,7 @@
.react-datepicker__month--accessible:focus {
background: $euiFocusBackgroundColor;

.react-datepicker__day--in-range {
.react-datepicker__day--in-range:not(.react-datepicker__day--selected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixes this issue:

Screen Shot 2019-06-14 at 11 28 19 AM

now:

@@ -280,11 +280,12 @@ EuiDatePicker.propTypes = {

EuiDatePicker.defaultProps = {
adjustDateOnChange: true,
dateFormat: 'MM/DD/YYYY hh:mm A',
dateFormat: 'MM/DD/YYYY',
Copy link
Contributor

Choose a reason for hiding this comment

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

It didn't make sense that the default date format would show time when by default, showTimeSelect is false. So I removed the time format just from this default value.

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.

I approve this now with the added changes I made. 😄

@cchaos cchaos requested a review from thompsongl June 14, 2019 15:31
@cchaos
Copy link
Contributor

cchaos commented Jun 14, 2019

@snide I can't request you as a reviewer since you're the creator, but can do take a look at my changes?

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.

📅 LGTM

@cchaos
Copy link
Contributor

cchaos commented Jun 14, 2019

@thompsongl Can you actually check my last commit as well: 61d463e

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.

Still looks good

@thompsongl
Copy link
Contributor

This one is ready to merge after updating the CL, yeah?

@snide
Copy link
Contributor Author

snide commented Jul 2, 2019

@thompsongl yep. ill update and get it merged.

@snide
Copy link
Contributor Author

snide commented Jul 2, 2019

jenkins test this

@snide snide merged commit e4db15d into elastic:master Jul 2, 2019
@snide snide deleted the bug/resp_dp branch July 2, 2019 19:28
@snide snide restored the bug/resp_dp branch July 2, 2019 19:28
@snide snide deleted the bug/resp_dp branch July 2, 2019 19:28
@snide snide restored the bug/resp_dp branch July 2, 2019 19:28
@snide snide mentioned this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiDatePicker's container is wrong on small screens
3 participants