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

Start date popover for date picker range starts at the calendar icon #3383

Merged
merged 8 commits into from
Apr 29, 2020

Conversation

winnllam
Copy link
Contributor

@winnllam winnllam commented Apr 24, 2020

Summary

Fixes #3374
Icon has been changed to be a part of the start date, so the picker aligns with the front,
datePickerRange

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@elizabetdev elizabetdev self-requested a review April 24, 2020 10:21
@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

@winnllam, thanks for creating this PR.

The way you fixed this issue introduces a new issue. If we pass, for instance, iconType="cheer" to the EuiDatePickerRange this is what happen. Two icons are displayed:

Screenshot 2020-04-27 at 18 37 47

As you can see, just changing the startControl showIcon: true enables the calendar icon.

But if you <EuiDatePickerRange iconType={false} /> it doesn't remove the icon:

Screenshot 2020-04-27 at 18 51 33

And if you <EuiDatePickerRange iconType={true} /> two icons are displayed:
Screenshot 2020-04-27 at 18 50 42

@cchaos
Copy link
Contributor

cchaos commented Apr 29, 2020

Hey @winnllam, jumping in here...

This is a great, easy solution. The nice thing is that flex is still sizing the inputs correctly too. It actually looks much better, spacing-wise, than the previous solution.

Before
Screen Shot 2020-04-28 at 21 39 59 PM

After
Screen Shot 2020-04-28 at 21 39 34 PM


I pushed a commit to your fork directly. The logic was just a little off and I wanted to align the prop name with our typical icon prop naming convention iconType instead of optionalIcon. Now consumers can also customize this icon via the iconType prop directly on the EuiDatePickerRange or EuiDatePicker itself.

Screen Shot 2020-04-28 at 21 47 23 PM

Screen Shot 2020-04-28 at 21 54 43 PM

The icon can also be removed entirely.

Screen Shot 2020-04-28 at 21 56 02 PM

The extra logic you added in EuiDatePickerRange for deciding which icon to display was duplicative of the EuiDatePicker handling it itself, so I deleted that bit and just passed down the show/don't show prop.

Overall, great solution! Thanks so much.

One more screenshot showing the actual popover:

Screen Shot 2020-04-28 at 21 55 57 PM

@cchaos cchaos requested a review from elizabetdev April 29, 2020 02:13
@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

I looked at the code and tested locally.

Thanks @winnllam for helping us out with this issue and thanks @cchaos for jumping in!

@elizabetdev elizabetdev merged commit fd4ee90 into elastic:master Apr 29, 2020
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.

[EuiDatePickerRange] StartDateControl popover wrong placement
4 participants