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

[EuiDatePicker] Update styles for Amsterdam #5000

Merged
merged 15 commits into from
Aug 19, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 30, 2021

This PR only affects the Amsterdam theme by separating out the date-picker imports completely and kinda hacky. But I wanted to start clean and not try to "override" the default theme because it's soo many selectors.

The updated version:
Screen Shot 2021-07-30 at 15 46 29 PM

The main differences are:

  • Month/Year selectors act like dropdowns instead of the grid selector
  • The Month/Year header expands the whole width, even over the time selection
  • Using sort arrows for cycling through months
  • Updates to the styling of the selected days including hover/focus states and especially when in a range

Screen Shot 2021-08-11 at 16 13 07 PM

  • Stacks time below calendar on mobile (I know it's awkward, but its the best I could do for now).

Screen Shot 2021-08-11 at 16 14 20 PM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs and playground toggles
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs 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

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

@kibanamachine
Copy link

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

@cchaos cchaos changed the title [WIP] [EuiDatePicker] Update styles for Amsterdam [EuiDatePicker] Update styles for Amsterdam Aug 11, 2021
Comment on lines -226 to -227
<span>
<span className={classes}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I just removed the extra plain wrapping <span> that I could not figure out why was here.

@cchaos cchaos marked this pull request as ready for review August 11, 2021 20:18
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

I like these changes 💟

One thing I noticed: the focus state for time selection when navigating via keyboard is kind of hard to see (1:00 PM):

image

That focus color is pretty close to the base background color. Have any ideas on increasing the contrast or adding other visibility?

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.

The changes are looking great! 😍

I just found a few design issues. Going to list them by number so it is easier to track them.

1 - When focusing the time there are multiple black outlines. It only happens in Chrome and Edge. I was expecting to have a black outline to all the popover and then when pressing "arrow up" or "arrow down" just to have one focus state on the currently focused item.

Screenshot 2021-08-16 at 18 44 21

2 - The months and years dropdowns grow in width when hovering the time items. It only happens in Safari.

Screen.Recording.2021-08-16.at.07.06.PM.mov

3 - The black focus outlines have some inconsistencies. Is it possible to both date and time popovers to have similar outlines? The outline on "months" looks better to me.

date-picker@2x

@cchaos
Copy link
Contributor Author

cchaos commented Aug 17, 2021

I have no idea what's goin on with those focus rings on the time selection. It should def not highlight all those items. 👀

@miukimiu , good spot on those other two issues and yes, unfortunately, they are known and a symptom of the library.

  1. I cannot for the life of me figure out why this is happening other than the fact that react-datepicker does some calculations on the size of that container that I think interferes with the overflow. It is just Safari, and so I'm keen to just leave it and hope we'll re-write this component soon. In summary, its a known/understood regression.

  2. Yes, the focus ring is on different elements for the month vs year dropdown. This is because, terribly, the react-datepicker orders the years in reverse by default. So 2030 -> 2029 -> 2028 etc, vs how we like to read from top to bottom 2014 -> 2015 -> 2016. So I have to use flex-direction: column-reverse to visually re-order the list, but because of how flex reacts with overflow I had to do this an entirely different way from the month. UGH!

@cchaos
Copy link
Contributor Author

cchaos commented Aug 17, 2021

😅 Ok I pushed a fix to the time focus states. I had to dig deeper and find that because of moving where the overflow scroll was happening changed the browser's interpretation of :focus and therefore was applying it to wrong items. So.. I had to move the overflow back to the correct container and then also add an underline to the preselected class to fake focus while the whole list gets the actual focus ring...

Screen Shot 2021-08-17 at 15 12 05 PM

@kibanamachine
Copy link

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

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.

😅 Ok I pushed a fix to the time focus states

Thanks for making those changes. I tested again and LGTM! 🎉

@cchaos cchaos merged commit 0a9857d into elastic:master Aug 19, 2021
@cchaos cchaos deleted the ams/date_picker branch August 19, 2021 15:15
cchaos added a commit that referenced this pull request Aug 20, 2021
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