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

me: Notifications: Stop notices from building up on screen #26943

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

alisterscott
Copy link
Contributor

@alisterscott alisterscott commented Aug 28, 2018

Ensures that notices on the Me/Notifications screen don't build up unnecessariy

Fixes #3203

Before

before

After

after

Testing Instructions

  1. Visit /me/notifications
  2. Make some changes and click save, then make more changes click save
  3. Ensure that notices don't build up on screen
  4. Ensure that after ~4 seconds the success notice automatically dismisses

@alisterscott alisterscott added [Type] Enhancement [Feature] Notifications Site notifications. [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. labels Aug 28, 2018
@alisterscott alisterscott self-assigned this Aug 28, 2018
@matticbot
Copy link
Contributor

@alisterscott alisterscott changed the title me: Notifications: Stop notices from piling up on screen me: Notifications: Stop notices from building up on screen Aug 28, 2018
@alisterscott alisterscott added FixTheFlows [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 28, 2018
@alisterscott alisterscott requested review from rachelmcr and a team August 28, 2018 23:32
Copy link
Contributor

@aidvu aidvu left a comment

Choose a reason for hiding this comment

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

LGTM!

@aidvu aidvu added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 29, 2018
Copy link
Contributor

@Stojdza Stojdza left a comment

Choose a reason for hiding this comment

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

Works as expected, mobile and desktop.

Copy link
Member

@rachelmcr rachelmcr left a comment

Choose a reason for hiding this comment

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

Looks great! While testing I found a new issue #26949 but that predates this PR.

@hoverduck
Copy link
Contributor

It works well on the /me/notifications page, but not elsewhere in /me. Would it be better to have a more general solution?

@alisterscott alisterscott merged commit 5bb7ee7 into master Aug 30, 2018
@alisterscott alisterscott deleted the fix/notification-settings-confirmation branch August 30, 2018 00:05
@alisterscott
Copy link
Contributor Author

It works well on the /me/notifications page, but not elsewhere in /me. Would it be better to have a more general solution?

I'll open a new PR for this

@alisterscott
Copy link
Contributor Author

It works well on the /me/notifications page, but not elsewhere in /me. Would it be better to have a more general solution?

@hoverduck do you have specific examples? I can't seem to find any testing this - only that some don't automatically disappear...

@hoverduck
Copy link
Contributor

@alisterscott On further inspection this might not be that big of a deal. The alerts don't disappear, but they also don't stack up like the ones you fixed in this PR. It's maybe not ideal, but it's not nearly as irritating. This is what I saw editing any of the profile fields on the main /me page:

wp-calypso-me-notifications-stay

@Stojdza
Copy link
Contributor

Stojdza commented Aug 30, 2018

It looks nice when notice is refreshed after second Save, that's not the case on /me/notificatios screen. That way user is sure that saving was successful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Notifications Site notifications. [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. FixTheFlows [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants