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.

Sign up for GitHub

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

Jump to bottom

Issue/10649 material reader post #11166

Merged
merged 17 commits into from
Feb 12, 2020
Merged

Issue/10649 material reader post #11166

merged 17 commits into from
Feb 12, 2020

Conversation

khaykov
Copy link
Member

@khaykov khaykov commented Jan 26, 2020

Fixes #10649

This PR is based on #11164, which needs to be merged first.

This PR moves reader post details and nested screens to the material theme and adds a dark mode support.

comparison_reader_post

To test:

  • Navigate to post details, post comments, and list of users who liked it. Try to like/bookmark posts and make sure everything looks and works ok in both light and dark themes.
    The theme can be changed 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 Reader [Status] Needs Design Review A designer needs to sign off on the implemented design. Dark Mode labels Jan 26, 2020
@khaykov khaykov added this to the 14.2 milestone Jan 26, 2020
@khaykov khaykov requested a review from mattmiklic January 26, 2020 01:22
@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 26, 2020 •
edited
Loading

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

mattmiklic
mattmiklic approved these changes Jan 27, 2020
View reviewed changes
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.

Overall this looks great. Just a couple of things to followup on:

  • the author name and site title at the top of the screen should use the High Emphasis On Surface color instead of blue. The follow icon/text should remain blue since it's actionable.
  • Double-check that we're using the High Emphasis On Surface color for post titles and body text as mentioned here.

@khaykov
Copy link
Member Author

khaykov commented Jan 29, 2020

@mattmiklic I updated the color to use high type alpha 👍
Btw, the author's name and site name are actionable - tapping on it opens their blog. Do you still want to use a high type onSurface for them?

comparison_post_detail_text_color

@mattmiklic
Copy link
Member

I think that's ok... tapping the user's name isn't the primary action, so I don't think we want to make it compete with the Follow action visually. That latest screenshot looks good to me.

@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
11f4259 
…WordPress-Android into issue/10649-material-reader-post
@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 31, 2020
@develric develric self-assigned this Feb 5, 2020
develric
develric approved these changes Feb 5, 2020
View reviewed changes
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.

Hi @khaykov 👋! I tested the app in both light and dark theme on the screens reported above and it looks good to me! I just left a really super minor np comment in line, but this said I'm approving this so feel free to ignore that one and merge 😊!

WordPress/src/main/res/values/dimens.xml Outdated
Comment on lines 20 to 21
<item name="primary_button_disabled_alpha" format="float" type="dimen">@dimen/disabled_alpha
</item>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I remember this is the way AS auto format this sort of things and this is less than a np, it's more a proof that I went into the code 😬! I would align on a single line, but really super minor so feel free to just ignore this!

Copy link
Member Author

@khaykov khaykov Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I switched formatting to multiline, otherwise AS will reformat it again in the future :)

@khaykov khaykov removed the request for review from Gio2018 February 5, 2020 22:52
@jkmassel
Copy link
Contributor

We're freezing 14.2 today, so this PR is being bumped to 14.3. If you'd like it to ship with 14.2, please merge it into release/14.2 and ping me – I'll be happy to cut a new beta release.

@jkmassel jkmassel modified the milestones: 14.2, 14.3 Feb 10, 2020
@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
64f6fc6 
…WordPress-Android into issue/10649-material-reader-post

# Conflicts:
#	WordPress/src/main/res/values/dimens.xml
@khaykov khaykov merged commit dc26e26 into feature/material-theme Feb 12, 2020
@khaykov khaykov deleted the issue/10649-material-reader-post branch February 12, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@develric develric develric approved these changes

@mattmiklic mattmiklic mattmiklic approved these changes

@theck13 theck13 Awaiting requested review from theck13

@malinajirka malinajirka Awaiting requested review from malinajirka

@nbradbury nbradbury Awaiting requested review from nbradbury

@mzorz mzorz Awaiting requested review from mzorz

Assignees

@develric develric

Labels
Reader [Status] Needs Code Review
Projects
None yet
Milestone
14.3
Development

Successfully merging this pull request may close these issues.
4 participants
@khaykov @mattmiklic @jkmassel @develric