Skip to content

Issue/10651 material notifications list and settings#11192

Merged
khaykov merged 14 commits intofeature/material-themefrom
issue/10651-material-notifications-list
Feb 13, 2020
Merged

Issue/10651 material notifications list and settings#11192
khaykov merged 14 commits intofeature/material-themefrom
issue/10651-material-notifications-list

Conversation

@khaykov
Copy link
Copy Markdown
Contributor

@khaykov khaykov commented Jan 30, 2020

Part of #10651

This PR moves notifications list and settings to the material theme and adds a Dark Mode support.

For unread notifications, we are using the same bg color as for multi-selected sites in Site Picker.

comparison_notification_list

To test:

  • Navigate to the Notifications tab and make sure that they look ok in both light and dark themes.
  • Navigate to Notification settings and confirm that they look and behave as expected in both light and dark themes.

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 Notifications [Status] Needs Design Review A designer needs to sign off on the implemented design. Dark Mode labels Jan 30, 2020
@khaykov khaykov added this to the 14.2 milestone Jan 30, 2020
@khaykov khaykov requested a review from mattmiklic January 30, 2020 08:10
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 30, 2020 •
edited
Loading

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

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 30, 2020

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

Generated by 🚫 dangerJS

@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
02eb180 
…WordPress-Android into issue/10651-material-notifications-list
@khaykov
Merge branch 'feature/material-theme' of github.com:wordpress-mobile/…
acd1bb3 
…WordPress-Android into issue/10651-material-notifications-list
@khaykov khaykov mentioned this pull request Feb 2, 2020
Merged
3 tasks
mattmiklic
mattmiklic approved these changes Feb 3, 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 really good. I just see one small issue; there's a white border around the icon that's overlaid on top of the user's avatar; in the dark theme, we should switch that border color to the surface color I believe, so it's the same color as the default (almost-black) notification background.

@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Feb 4, 2020

@mattmiklic sure! I updated the color :)

comparison_noticon

@mattmiklic
Copy link
Copy Markdown
Member

mattmiklic commented Feb 5, 2020

Looking good!

@khaykov khaykov removed the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Feb 7, 2020
@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
renanferrari
renanferrari reviewed Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

I left some comments with some minor nitpicks and potentially unrelated questions.

@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Feb 12, 2020

@renanferrari thanks for the review! I hope I addressed your concerns :)

renanferrari
renanferrari approved these changes Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@khaykov khaykov merged commit b7302af into feature/material-theme Feb 13, 2020
@khaykov khaykov deleted the issue/10651-material-notifications-list branch February 13, 2020 01:11
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

@jd-alexander jd-alexander Awaiting requested review from jd-alexander

@malinajirka malinajirka Awaiting requested review from malinajirka

@develric develric Awaiting requested review from develric

@marecar3 marecar3 Awaiting requested review from marecar3

+1 more reviewer

@renanferrari renanferrari renanferrari approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Notifications

Projects

None yet

Milestone

14.3

Development

Successfully merging this pull request may close these issues.

4 participants

@khaykov @mattmiklic @jkmassel @renanferrari