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

Polish Date Range picker UI #13579

Merged
merged 8 commits into from
Dec 16, 2020
Merged

Polish Date Range picker UI #13579

merged 8 commits into from
Dec 16, 2020

Conversation

malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Dec 10, 2020

Parent issue #13268

Not ready for code review - but feel free to share ideas :).

This PR updates styles of Date Range picker in Activity Log.

Reply to this comment:

The calendar on tablet has a split view. Think we should make the background of the area on the left white rather than blue, and make the text black

I went with a fullscreen view even on tablets as discussed.

  • Future dates: should we grey out dates after today and make them unselectable?
  • Calendar on mobile: main divider is heavy black - should be grey if possible.

Fixed

  • On mobile, should we show the week with today's date near the bottom of the view on first load? Maybe we can't control that.

Partially fixed - we show the current month at the bottom.

  • I didn't have the divider below days of the week, but we can keep it too if it can be grey.
  • Wonder if the blue bar which connects the two dates can go full width of the screen, would look cool. Maybe not possible.
  • Currently the "Start date" or "End date" text is always black. Should be greyed out before its selected
  • Wasn't in the designs, but feels like maybe we should have a 'CLEAR' action top right here, like we do in Activity Type. I know I can clear with the chip itself. But if I have the calendar open its feels weird that I can't make it unselected

AFAIK these changes are not supported by default. Having said that if we really want to implement them there might be some workarounds. It'd be good to know priority so we can better timebox it.

Screenshot 2020-12-10 at 13 49 20

Questions:

  1. I got stuck on changing the color of the status bar in styles.xml. I think I'm missing something obvious, any ideas?
  2. If you think anything of the above is feasible, please let me know. I think some of the things would be doable programatically, but it feels hacky and unreliable (the component is a final class).

To test:

  1. Test the Activity Log Date Range filter - old api, new api, tablet, dark mode

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@malinajirka malinajirka added this to the 16.5 milestone Dec 10, 2020
@malinajirka malinajirka added [Status] Needs Design Review A designer needs to sign off on the implemented design. [Status] Not Ready for Review labels Dec 10, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 10, 2020

You can test the changes on this Pull Request by downloading the APK here.

@osullivanchris
Copy link

Tested light mode, dark mode, landscape, portrait, phone, tablet.

Looking and working great. Minor details

  • the calendar on light mode is all on white background. But on dark, there's a black and grey background. Wonder is the grey needed? I think all black is probably fine.
  • on a super small device, should we be worried about the size of the header? Might be ok. It doesn't take up any more space than pages where we have a combination of bottom tabs and top nav.
  • No date selected. Opened calendar, made a date selection, then without saving yet, rotated the device. It cleared my dates. On emulator/simulator

I went with a fullscreen view even on tablets as discussed.

This is fine for me, right now.

Fixed

Thanks, looks good

Partially fixed - we show the current month at the bottom.

I think this is the right balance. Let's stick with it!

Of the issues that you couldn't fix, here would be my priority order:

  1. 'Clear' action next to 'save'. Functional. But achievable in another way for the user if stuck.
  2. "Start date" "End date" in grey - feels like content when its just a placeholder
  3. Black line - only a visual/taste thing. Not going to affect the user.
  4. Blue line going full-width - totally just my taste. Not a big deal. Also might be tricky - doing it will look weird on tablet/landscape, so we'd have to have exceptions. Might scrap this one altogether.

So scrap 4. 1 is somewhat functional. 2 and 3 visual nice to haves. I wouldn't block the release on any necessarily.

thanks

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@malinajirka
Copy link
Contributor Author

Thanks for the feedback @osullivanchris!

the calendar on light mode is all on white background. But on dark, there's a black and grey background. Wonder is the grey needed? I think all black is probably fine.

Updated
Screenshot 2020-12-16 at 09 46 47

on a super small device, should we be worried about the size of the header? Might be ok. It doesn't take up any more space than pages where we have a combination of bottom tabs and top nav.

It'd usually make concern me as well. But it's an official component and we can't do much about it anyway.

No date selected. Opened calendar, made a date selection, then without saving yet, rotated the device. It cleared my dates. On emulator/simulator

I was not able to reproduce this issue. It works as expected on my emulator.

'Clear' action next to 'save'. Functional. But achievable in another way for the user if stuck.

The class is final and it doesn't support customizing the actions. We could potentially implement a very hacky solution, but it might break with every release of the component. So I wouldn't do anything unless users complain about it.

"Start date" "End date" in grey - feels like content when its just a placeholder

Similar as above. The component uses a single textview which is not accessible from our code. The solution for this one would be even more hacky.

The customization of the component is very limited and not straight-forward at all. If we decide that we need some of the above adjustments I'd recommend using a 3rd party library or implementing a custom component which would be fully customizable.

@malinajirka malinajirka requested review from zwarm and removed request for zwarm, khaykov and ashiagr December 16, 2020 08:53
@malinajirka malinajirka removed [Status] Not Ready for Review [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Dec 16, 2020
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

Nicely done @malinajirka - especially with the constraints when using the gated component. I took it for a spin on the following emulator types: tiny device, tablet, and modern. I also test on my Pixel 3XL device. 👍

@zwarm zwarm merged commit 3ff280b into develop Dec 16, 2020
@zwarm zwarm deleted the issue/13268-polish-calendar-ui branch December 16, 2020 15:44
@malinajirka malinajirka mentioned this pull request Dec 17, 2020
3 tasks
@osullivanchris
Copy link

@malinajirka thanks for the clarifications in the comment above. Think we're on the same page on this one. You have all my questions covered and it sounds like we've got what we can for now. Other enhancements can be parked. Not worth the trouble.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants