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

Issue/10649 material reader post list #11164

Merged
merged 28 commits into from
Jan 31, 2020

Conversation

khaykov
Copy link
Member

@khaykov khaykov commented Jan 25, 2020

The first part of #10649

This PR moves Reader Post List and Tags & Sites to Material Theme and adds Dark Theme support. There are some code changes for cosmetic reasons.

@develric @osullivanchris I would like to loop you guys in since this PR touches Subfilter, which is part of IA.

Reader without IA changes:

comparison_reader_no_ia

With IA changes (alpha branch):

comparison_reader_post_list

To test:

  • Navigate to Reader post list, and switch between different filters/subfilters. Make sure everything looks good in both light and dark themes.
  • Navigate to Reader's Tags & Sites screen, and make sure everything looks good in both light and dark themes.

You can switch the theme from App Settings.

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.

@khaykov khaykov added [Status] Needs Design Review A designer needs to sign off on the implemented design. IA labels Jan 25, 2020
@khaykov khaykov added this to the 14.1 milestone Jan 25, 2020
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 25, 2020

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

@khaykov
Copy link
Member Author

khaykov commented Jan 26, 2020

Forgot to update selectors for like/comment buttons:
Image from Gyazo

Image from Gyazo

@khaykov khaykov mentioned this pull request Jan 26, 2020
3 tasks
Copy link
Member

@mattmiklic mattmiklic left a comment

Choose a reason for hiding this comment

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

So many little visual improvements here, even in the light mode, by switching to the material theme. <3

Two small things I see:

  • The round avatars are showing up with a white square background in dark mode; I'm guessing that should be fixable because I think we're rounding them in the app
  • The share and block icons in the popup menu are black/white, can we drop those back to "medium emphasis" (or whatever we've done to icons elsewhere) to make them recede visually just a bit?

@maxme maxme modified the milestones: 14.1, 14.2 Jan 27, 2020
Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

thanks for the ping! Looks good to me and no conflicts with the reader work @develric and I are doing. Good for him to be aware though as we still have new UI to introduce, that the new elements use the material theme.

Only general piece of feedback which is particularly noticeable in the reader where you're doing longer form reading - I find the absolute white on absolute black a little stark to read in large chunks. I know we are using system defaults but that's just the feeling I have. Maybe its due to the serif font that it becomes an issue.

@mattmiklic
Copy link
Member

mattmiklic commented Jan 27, 2020

I find the absolute white on absolute black a little stark to read in large chunks.

@khaykov could you double-check that we're using the High Emphasis On Surface color for the titles and body text in the Reader? In dark mode it should be 87% #FFFFFF so not quite as stark as full white on full black.

(ref: pauD4L-r0-p2)

newcolors-dark-1

…WordPress-Android into issue/10649-material-reader-post-list

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderPostListFragment.java
@khaykov
Copy link
Member Author

khaykov commented Jan 29, 2020

@mattmiklic I fixed avatar BG, updated post title, excerpt and menu icon colors to use High Emphasis alpha 👍
comparison_reader_text

@mattmiklic
Copy link
Member

I think that looks great. 👍

@khaykov khaykov added [Status] Needs Code Review and removed [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Jan 29, 2020
@osullivanchris
Copy link

osullivanchris commented Jan 29, 2020

Looks good!! One other small thing I'm noticing is the stroke around images (on posts in the reader feed). I am thinking we have a stroke for when an image is mostly white on white, or black on black, to show where the image ends and not just have something floating.

However the stroke looks a bit strong to me. Should we use less emphasis?

@mattmiklic
Copy link
Member

However the stroke looks a bit strong to me. Should we use less emphasis?

Good catch, that should probably be darker/more subtle in dark mode.

@khaykov
Copy link
Member Author

khaykov commented Jan 29, 2020

I totally missed that one! I changed the color to use disabled emphasis (medium one looks a bit too strong in light mode). WDYT?

comparison_reader_border

@mattmiklic
Copy link
Member

That looks better for sure. Would it be possible to use the same color we use for the dividers between posts? It's a little more subtle and it'd be nice if it matched.

@khaykov
Copy link
Member Author

khaykov commented Jan 29, 2020

@mattmiklic Sure thing! Looks like this:
comparison_reader_text

@mattmiklic
Copy link
Member

I think that's just right. 👌

Copy link
Contributor

@develric develric left a comment

Choose a reason for hiding this comment

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

Looked into the code and tested both light and dark theme. Worked as expected and looks really cool 😎! (I like we moved to snackbars in the settings activity! 👍)

Only thing I noticed is we reduced the height of the EditText where you can add a url or tag (see bottom of image below)

image

I see it is something also captured in your images so I guess it is fully wanted. If this is the case just ping me here and I will approve/merge 👍!

@khaykov
Copy link
Member Author

khaykov commented Jan 31, 2020

Thanks for taking a look at this PR, @develric ! I tried to make the bottom text area look more like one used in comments, so it's a bit shorter, so it looks as expected :)

@khaykov khaykov merged commit edc997f into feature/material-theme Jan 31, 2020
@khaykov khaykov deleted the issue/10649-material-reader-post-list branch April 15, 2020 16:58
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.

5 participants