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

Site Settings: Display global error/success notices when deleting a site #3076

Merged
merged 6 commits into from
Feb 5, 2016

Conversation

drewblaisdell
Copy link
Contributor

Fixes #158 by showing the following notice when users try to delete a site with active purchases.

screen shot 2016-02-05 at 11 25 17

Pinging @mikeshelton1503 for a design review.

Testing

  • Visit /settings/delete-site/:site for a site with active subscriptions.
  • Attempt to delete the site.
  • Assert that you see the error notice above.
  • Visit /settings/delete-site/:site for a site without active subscriptions.
  • Attempt to delete the site.
  • Assert that you see a success message when the site is deleted.
  • Product review
  • Code review

@drewblaisdell drewblaisdell self-assigned this Feb 4, 2016
@drewblaisdell drewblaisdell force-pushed the fix/delete-site-notices branch from 67c8461 to 0366970 Compare February 4, 2016 21:38
@scruffian scruffian added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Feb 5, 2016
@rickybanister
Copy link

It was never really intended to have the dismiss button to the right of the notice-action.

Can't this be an inline notice, or a modal dialog?

@mikeshelton1503
Copy link
Contributor

I am planning to spend time next week on fixing "long notices" and situations like this. However I'll go ahead and post thoughts here so we can get this done today.

I agree with Rick that dismiss button doesn't work well on the right. How about making the dismiss action a text link instead and shortening the copy:

image

@rickybanister
Copy link

I might be confused still, but I don't think you need a 'dismiss' link at all since you can't proceed with deletion without first removing your upgrades.

Will this notice follow you around everywhere or only be displayed on the delete page? If it is only displayed when viewing the deletion options I think it's fine to remove the dismiss link as navigating away from there implies you got cold feet and are cancelling your deletion.

@danhauk
Copy link
Contributor

danhauk commented Feb 5, 2016

I think the inline "Dismiss" link works better than the gridicon on the right side of the notification. However, my first thoughts tend to agree with @rickybanister that we should think about whether the dismiss link is really necessary in this case.

The use case for this notice is pretty limited and I would personally rather see it simplified with just one action.

But +100 to shorter copy.

@scruffian
Copy link
Member

@mikeshelton1503 - how's this?

screen shot 2016-02-05 at 17 12 52

@mikeshelton1503
Copy link
Contributor

Ok chatting with @rickybanister and @scruffian I understand this case a little better now and have a revised proposal :)

So users with active Purchases will only reach the Delete page if they click "Delete Site" from wp-admin (we are redirecting to Calypso to fix a separate issue), so we're talking about a smaller percentage of users. Otherwise, within Calypso a user will still be shown the modal window on the Settings page when they click "Delete Site" there.

So for this case, let's instead disable the "Delete Site" button and show the notice inline. There's no need in showing an action the user can't actually perform. We should go ahead and inform of what needs to happen in order to delete via an inline notice.

@scruffian pointed out to me that even with that solution in case of error and we mistakenly show the "Delete Site" button we should still show this notice. So 👍 to this. We can open a separate PR to handle the inline notice and button disabling.

@mikeshelton1503 mikeshelton1503 removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Feb 5, 2016
@scruffian
Copy link
Member

Code looks good 👍

@scruffian scruffian 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 Feb 5, 2016
@scruffian scruffian force-pushed the fix/delete-site-notices branch from a027f93 to 865617f Compare February 5, 2016 17:25
scruffian added a commit that referenced this pull request Feb 5, 2016
Site Settings: Display global error/success notices when deleting a site
@scruffian scruffian merged commit 86228f5 into master Feb 5, 2016
@scruffian
Copy link
Member

let's instead disable the "Delete Site" button and show the notice inline. There's no need in showing an action the user can't actually perform. We should go ahead and inform of what needs to happen in order to delete via an inline notice.

I created a new issue for this : #3117.

@scruffian scruffian deleted the fix/delete-site-notices branch February 5, 2016 17:36
@rralian
Copy link
Contributor

rralian commented Feb 5, 2016

@drewblaisdell @scruffian I don't think this addresses #158, or at least not terribly well. That issue was about showing the success message after deleting a site. Because deleting a site necessarily redirects you to a page other than the settings page of your deleted site, it would be best to show the success message on that redirected page. You are currently showing the success message on the settings page for a split-second before redirecting to stats, which isn't really long enough for people to read it or feel assured of their action.

@rickybanister
Copy link

If a user deletes their only site where are they directed?

@adambbecker
Copy link
Contributor

@rickybanister: Yeah I was thinking about that as well. I think we could do something like this:

delete_start-over

Obviously those bottom cards could be expanded upon, but for now probably just the top card could do the trick. What do you think?

If we're looking for a shorter term thing, I think the reader with the same notice could be fine for the time being.

@mikeshelton1503
Copy link
Contributor

I don't think this addresses #158, or at least not terribly well.

I agree with @rralian that this didn't address #158 Sorry I should have noticed this. It fixed an issue just not that one.

If a user deletes their only site where are they directed?

I tested this and was shown this:

image

However I'll note that after I clicked Delete I saw a success notice that said my site was being deleted and the Delete Site button was disabled but after waiting a few minutes I never got redirected anywhere. Only when I refreshed the page did I see the above screen.

I like @adambbecker's suggestion for what to show there if no site's exist on the account after deletion. I create a new issue for that since it's a separate issue: #3126

@rickybanister
Copy link

Yeah, @adambbecker that would be pretty great. I think the two cards (create a site and follow people) could probably be the same size if you do the same sort of treatment to each for the illustration. But that's a nice idea.

We could use a url like /start/fresh or something. I think nux uses '/start' right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings. [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants