Skip to content

Issue/10648 material web preview#11169

Merged
malinajirka merged 9 commits intofeature/material-themefrom
issue/10648-material-web-preview
Feb 10, 2020
Merged

Issue/10648 material web preview#11169
malinajirka merged 9 commits intofeature/material-themefrom
issue/10648-material-web-preview

Conversation

@khaykov
Copy link
Copy Markdown
Contributor

@khaykov khaykov commented Jan 27, 2020

Fixes #10648

This PR moves Web Preview screens to the material theme and adds dark mode support.

comparison_webpreview

To test:

  • Check Site, Post and Theme previews and make sure they look 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 [Status] Needs Design Review A designer needs to sign off on the implemented design. Previews labels Jan 27, 2020
@khaykov khaykov added this to the 14.2 milestone Jan 27, 2020
@khaykov khaykov requested a review from mattmiklic January 27, 2020 01:59
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 27, 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.

This is looking good.

It's not a major thing, but my understanding of elevation in dark themes is that the more elevated the surface, the lighter its background should be. But here, the popup menu sitting over the bottom toolbar has a darker background than the toolbar. It seems like the toolbar should be darker and the popup background lighter. I could be wrong though, what do you think @khaykov?

@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Jan 29, 2020

Great catch, @mattmiklic ! I also noticed that the bottom bar is too bright. I fixed elevation on both 👍

comparions_wp

@mattmiklic
Copy link
Copy Markdown
Member

mattmiklic commented Jan 29, 2020

Looking good now! 🚢

@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
@oguzkocer
Copy link
Copy Markdown
Contributor

oguzkocer commented Jan 30, 2020

@khaykov I don't think I can work on this PR this week since I am on groundskeeping. Hopefully someone will have time for it, but if nobody else takes it, I should be able to review it on Monday 🤞

@malinajirka malinajirka self-assigned this Jan 31, 2020
malinajirka
malinajirka reviewed Jan 31, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @khaykov ! Looks good overall.

@mattmiklic The "Refresh" button has different color from the rest of the buttons. When I opened the screen my first impression was that "UP" is disbabled. Is it a best practice to keep the "Refresh = Toolbar action buttons" different color than the "UP" button?

Is the "Desktop" label intentionally the same in both modes?

Light Dark
Screenshot_1580464128 Screenshot_1580464371

@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Feb 1, 2020

@malinajirka The UP button color was wrong. It's the only place we are using the white toolbar, so there was some mix up with styling. I updated it to be consistent with other screens (we are using onSurface color for Toolbar icons/text).

Image from Gyazo

I'll leave the question about "Desktop" label to @mattmiklic :)

@khaykov
Merge branches 'feature/material-theme' and 'feature/material-theme' …
cd3a8db 
…of github.com:wordpress-mobile/WordPress-Android into issue/10648-material-web-preview

# Conflicts:
#	WordPress/src/main/res/values/styles.xml
@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
731ee4b 
…WordPress-Android into issue/10648-material-web-preview
@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Feb 5, 2020

@malinajirka @mattmiklic mentioned that we can leave the label as is :)

@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Feb 10, 2020

Sorry, I missed the last update ;). LGTM, thanks!

@malinajirka malinajirka merged commit 769484e into feature/material-theme Feb 10, 2020
@malinajirka malinajirka deleted the issue/10648-material-web-preview branch February 10, 2020 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka left review comments

@mattmiklic mattmiklic mattmiklic approved these changes

@theck13 theck13 Awaiting requested review from theck13

@maxme maxme Awaiting requested review from maxme

@develric develric Awaiting requested review from develric

@oguzkocer oguzkocer Awaiting requested review from oguzkocer

Assignees

@malinajirka malinajirka

Labels

None yet

Projects

None yet

Milestone

14.2

Development

Successfully merging this pull request may close these issues.

4 participants

@khaykov @mattmiklic @oguzkocer @malinajirka