-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Activity type filter UI #13564
Activity type filter UI #13564
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far.
- High emphasis might be the wrong term. I can't find the correct reference right now. But I believe there's a native colour system now where the darkest color (high emphasis) is black at like 96% opacity. Secondary is black at 60%. However, if that's not what we're using. I'd just use the main black text color we use in the app, however its currently defined. In any case it looks correct in the build.
- Activity Type filter is basically perfect first time. Nice job 👍
Noting that this PR is on Activity Type. But I didn't realise and already took a bunch of notes on calendar!
- Calendar on Android tablet. I actually shared 2 examples in a post earlier. I saw this one in the guidelines too, and its one I left out. It seems to be working pretty well though. And clearly that's the one that comes out of the box. So I think we can go in this direction.
- 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
- Future dates: should we grey out dates after today and make them unselectable? 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
- Calendar on mobile: main divider is heavy black - should be grey if possible. 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.
- Noting that when I have two dates selected, and I tap a date again, it starts with the first date. I think this is correct. Behaviour as I would expect.
- 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
- When the user saves a filter...either Activity Type or date range. Do we keep it forever? For this session?
Thanks so much! ;) I'll see what I can do. I'll create a separate PR for the Calendar component UI when it's ready.
We keep it until the user leaves the Activity Log screen. |
nice!
makes sense! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Everything is good codewise.
Just one observation: there's a padding around checkbox because of the ripple effect in the selected state and min touch target size so the actual padding between the checkbox and the text might not be exactly 32dp
. I think it is not easily customisable + we'd like to retain min touch target size, so all 🟢 from my side.
Parent issue #13268
I applied the styles per the design comments and some things don't match (text color, line height, divider color , ...).
What color do you mean by "high emphasis" - could you please share the hex code so I can check if we have it defined in the code. Thanks!
This PR polishes the UI of the activity type filter screen
To test:
PR submission checklist:
RELEASE-NOTES.txt
if necessary.