Skip to content

Conversation

@renanferrari
Copy link
Contributor

Fixes #11468

This PR creates a new unelevated card style and applies it to the stats cards.

Phone (Light) Phone (Dark)
phone-light phone-dark
Tablet (Light) Tablet (Dark)
tablet-light tablet-dark

To test

Open the stats screen on a tablet and make sure it looks like the provided mockup (in the linked issue). Test on both light and dark modes.


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.

@peril-wordpress-mobile
Copy link

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

@iamthomasbishop
Copy link

iamthomasbishop commented Mar 19, 2020

@renanferrari @mattmiklic in dark theme, do you think the cards should be elevated to 1dp, similar to what’s shown in this example on the Material guidelines? Or are we assuming the cards are on surface level (0dp)?

Copy link
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

Looks great! Good job 👍 Feel free to merge this PR once you address @iamthomasbishop question

icon = R.drawable.ic_plus_white_24dp,
textResource = R.string.stats_management_add_new_stats_card,
navigationAction = Companion.create(this::onClick),
showDivider = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch 👍

@osullivanchris
Copy link

osullivanchris commented Mar 19, 2020

@iamthomasbishop @mattmiklic this looks fine to me! But just curious if we go away from cards and elevation would we consider going away from boxes too? If I was designing this page from scratch with no page background colour, I would probably do it with horizontal lines dividing the blocks of content, but not specifically put borders around the boxes, if that makes sense? Its a little bit more work so it could be a later step to be considered.

Here's a quick hack

borders to divs

@renanferrari
Copy link
Contributor Author

Or are we assuming the cards are on surface level (0dp)?

Hi @iamthomasbishop 👋 Yes, in this case we're assuming the cards are on surface level.

If I was designing this page from scratch with no page background colour, I would probably do it with horizontal lines dividing the blocks of content, but not specifically put borders around the boxes, if that makes sense? Its a little bit more work so it could be a later step to be considered.

Thank you for your input on this, @osullivanchris! From a development perspective, both options (with and without boxes) are quite simple to achieve, but having the same look on both phones and tablets is even simpler, which is the main reason I leaned towards the option presented here. @mattmiklic and @SylvesterWilmott also considered these options and they may have some thoughts to share about them, from a design perspective.

@planarvoid Thanks a lot for the review! 🙇‍♂️

@mattmiklic
Copy link
Member

Or are we assuming the cards are on surface level (0dp)?

if we go away from cards and elevation would we consider going away from boxes too

What's seen above is consistent with the Material Design rules for outlined cards. This felt like the most appropriate treatment for this usage, where there are many cards together on the same screen. Outlined cards have 0 elevation. So this is still a standard card treatment, just one that has a little bit more subtle visual style than elevated cards.

Re: @osullivanchris's mockup; that works for single-column views and it's consistent with how we use cards in the Reader and the Blog Posts screen. But since Stats includes a two-column view (on tablets, or on a phone when in landscape) this feels like the better approach to me.

@renanferrari renanferrari merged commit d272468 into feature/material-theme Mar 19, 2020
@renanferrari renanferrari deleted the issue/11468-stats-cards-bounds branch March 19, 2020 18:46
@iamthomasbishop
Copy link

What's seen above is consistent with the Material Design rules for outlined cards.

That makes sense, thanks @mattmiklic !

@osullivanchris
Copy link

Yep sounds good to me too!

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

Labels

[Status] Needs Code Review [Status] Needs Design Review A designer needs to sign off on the implemented design. [Type] Enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants