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

SuperDatePicker tweaks to make it selection more obvious #2049

Merged
merged 14 commits into from
Jun 20, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 14, 2019

This PR addresses no. 1 on the todo list from GAH:

Screen Shot 2019-06-14 at 15 48 26 PM copy

Added "Start date" and "End date" titles to the popover

Screen Shot 2019-06-14 at 17 26 43 PM

... as a tab because using the EuiPopoverTitle with tabs was not pretty

Screen Shot 2019-06-14 at 17 16 54 PM

Using bottom border to indicate which time period is selected

Similar to those used on the normal controls but I don't change the background to white so that hopefully indicated it's not editable in the button.

And for "needs update" state:

Screen Shot 2019-06-14 at 17 12 35 PM

And for invalid state:

Screen Shot 2019-06-14 at 17 10 43 PM

Added the title to tab aria-labels

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

@andreadelrio
Copy link
Contributor

LGTM. Only one minor thing, I'm not sure we need the colon next to START DATE and END DATE. Things already look a bit tight for the START DATE case.

@ryankeairns
Copy link
Contributor

I personally prefer the title separate from (on top of) the tabs 😬

As Andrea noted, it's pretty tight with the title in there and it sets a precedent of having the tab area share other content, which I don't think we've done elsewhere.

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.

I'll wait for design agreement here before doing my review

@cchaos
Copy link
Contributor Author

cchaos commented Jun 18, 2019

@ryankeairns The problems I have with using the default popover title are:

  1. It adds more visual height before you actually get to the contents of the popover
  2. The double borders from the title and tabs
  3. There's a lot of empty white space to the right of the title

Do you have any other suggestions besides either using the popover title or putting the title inline with the tabs?

@ryankeairns
Copy link
Contributor

@cchaos Maybe place the 'Start date' text in the tab panel and style it similar to an input label? To keep it looking a little more tidy (i.e. not floating in space), a little background color might pull it together while also making it a little more subtle:

(these colors are not EUI-y, I just approximated quickly in the Inspector)

Screenshot 2019-06-18 14 55 53

Screenshot 2019-06-18 14 56 01

Screenshot 2019-06-18 14 56 08

@cchaos
Copy link
Contributor Author

cchaos commented Jun 18, 2019

I'm wondering if the title needs to be as prominent now that there are also thick bottom borders to indicate which one is selected. Therefore, what if we just appended the "Start date" and "End date" to the inputs that live inside the tabbed content as well as in the now button like so:

@ryankeairns
Copy link
Contributor

The prepend solution looks good too!

cchaos added 7 commits June 18, 2019 19:08
- Fixed Absolute tab selection
- Using bottom border to indicate which time period is selected
- Added a “title” as a tab
- Added title to tab aria-labels
This reverts commit dac95c4.
- Also added `toSentenceCase` string service
@cchaos cchaos force-pushed the super-dp-tweaks branch from bd7664d to 3c661c0 Compare June 18, 2019 23:12
@cchaos
Copy link
Contributor Author

cchaos commented Jun 18, 2019

Alrighty, I updated the PR to use the prepend for the inputs:

Screen Shot 2019-06-18 at 18 25 23 PM

Screen Shot 2019-06-18 at 18 27 44 PM

Screen Shot 2019-06-18 at 18 27 50 PM

You'll notice, however, that the readonly state of under the Relative tab doesn't look readonly. We hadn't yet accounted for this readonly input group. I have another PR that I'm about to put up that will fix this.

@cchaos cchaos mentioned this pull request Jun 18, 2019
7 tasks
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

The prepend option looks much cleaner!

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

👍 yasss

@cchaos
Copy link
Contributor Author

cchaos commented Jun 19, 2019

jenkins test this

@cchaos
Copy link
Contributor Author

cchaos commented Jun 20, 2019

@thompsongl Would love to get this in before EOD Friday, if you've got the time.

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.

Code changes look good. Updated design looks great.

Thinking we need to add i18n to all these labels, right?

@cchaos
Copy link
Contributor Author

cchaos commented Jun 20, 2019

Yeah we do, but that's for another PR since this whole date picker is not internationalized

@cchaos cchaos merged commit 0ce3e6b into elastic:master Jun 20, 2019
@cchaos cchaos deleted the super-dp-tweaks branch June 20, 2019 20:09
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