Skip to content

Issue/10649 material reader post#11166

Merged
khaykov merged 17 commits intofeature/material-themefrom
issue/10649-material-reader-post
Feb 12, 2020
Merged

Issue/10649 material reader post#11166
khaykov merged 17 commits intofeature/material-themefrom
issue/10649-material-reader-post

Conversation

@khaykov
Copy link
Copy Markdown
Contributor

@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
Copy Markdown

peril-wordpress-mobile bot commented Jan 26, 2020

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Member

mattmiklic commented Jan 29, 2020

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
@khaykov
Removed unused resources.
2d25893
@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
8c58482 
…WordPress-Android into issue/10649-material-reader-post
@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
aa03cd7 
…WordPress-Android into issue/10649-material-reader-post
@develric develric self-assigned this Feb 5, 2020
develric
develric approved these changes Feb 5, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@develric develric Feb 5, 2020

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

jkmassel commented Feb 10, 2020

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

@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

+1 more reviewer

@develric develric develric approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@develric develric

Labels

Reader

Projects

None yet

Milestone

14.3

Development

Successfully merging this pull request may close these issues.

4 participants

@khaykov @mattmiklic @jkmassel @develric