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
Implement new “site cannot be accessed” flow #42682
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~2078 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~120 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
6f3937d
to
fb1a342
Compare
af12044
to
6b4e178
Compare
<FormattedHeader | ||
isSecondary | ||
align="left" | ||
headerText={ translate( 'Confirm your site is loading' ) } | ||
subHeaderText={ translate( | ||
'As a first step, we suggest trying to open your site to ensure it’s loading correctly.' | ||
) } | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i’ve used FormattedHeader here because CardHeading yields uneven spacing that’s annoying to fix, and i didn’t find a more appropriate component than those
site: { name: siteName, URL: siteUrl }, | ||
translate, | ||
} ) => { | ||
const confirmHref = `/settings/disconnect-site/confirm/${ siteSlug }?type=down`; | ||
|
||
let backHref = '/settings/manage-connection/' + siteSlug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug: the Back buttons on both old and new pages always take you back to Manage Connection (don’t worry about the reason branch below, it looks like a red herring), but if you used the CTA, that’s not necessarily where you came from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i make this onClick history.back + preventDefault and keep the href as a fallback, or is there a way to get the previous URL out of the router and set href to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be pretty cool if we can actually make it go back in history rather than dropping them in the Manage Connection page. Even though they won't be able to do much since the connection is broken, it would be nice that all the <- Back
links would behave in an expectable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple
import page from 'page';
page.back();
should be enough to go back.
Or window.history.back()
. The difference is that page.back()
will never navigate out of Calypso (i.e., to another page that was previously opened in the same tab.
If you want to construct a full <a href="..."/>
with a valid link, then the getPreviousRoute( state )
Redux selector will return the previous URL.
All of the above will work only if there is some navigation history to work with. If you got to the page by opening a direct link (from email, from chat, ...), clicking on the "back" arrow will do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow there are more options than i realised, and we could combine them in several ways, plus there’s also the question of whether this NavigationLink should go back through history (enabling the browser forward button) or push forward to a new history entry… does calypso have a convention i can defer to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does calypso have a convention i can defer to here?
I don't think so. Each of the techniques I described is used at least once in the codebase. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, thanks for rocking this PR out so quickly and efficiently, @delan ! Awesome work. Thank you for all your suggestions too!
This is looking really good to me, and only have a few notes:
- Design-wise, your color and styling changes make total sense to me.
- After testing this out, noticed that the copy at the top had the potential to be slightly confusing. Added a quick suggestion and also want to loop in @Automattic/editorial to ask for a quick validation on what we have here.
- I still have some questions about what options should open in new tabs and consequently what icons we should have there. Curious to know what others think about this, but it shouldn't hold us back. Let's ship and iterate afterwards.
- Mobile: the note at the bottom doesn't have any padding, stretching out to the edges of the frame (image 1).
- Mobile: I believe this might be the first time we're adding icons like this, so the margins on smaller screens look weird. On image 2 I added
margin-left: 0
to.disconnect-site__actions .card .formatted-header
. - Kudos for using real apostrophes! 🙌
Really awesome job, thanks for making this happen!
Image 1
Image 2
Before | After |
---|---|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've inspected the code and I don't see any blocking issues. Nice attention to detail 👍
{ translate( 'Remove Site' ) } | ||
{ siteIsAutomatedTransfer | ||
? translate( 'Remove Site' ) | ||
: translate( 'I’d like to fix this now' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice ’
punctuation!
|
||
const backHref = | ||
`/settings/disconnect-site/${ siteSlug }` + | ||
( type ? `?type=${ encodeURIComponent( type ) }` : '' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of encodeURIComponent
on the type
variable 👍
site: { name: siteName, URL: siteUrl }, | ||
translate, | ||
} ) => { | ||
const confirmHref = `/settings/disconnect-site/confirm/${ siteSlug }?type=down`; | ||
|
||
let backHref = '/settings/manage-connection/' + siteSlug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple
import page from 'page';
page.back();
should be enough to go back.
Or window.history.back()
. The difference is that page.back()
will never navigate out of Calypso (i.e., to another page that was previously opened in the same tab.
If you want to construct a full <a href="..."/>
with a valid link, then the getPreviousRoute( state )
Redux selector will return the previous URL.
All of the above will work only if there is some navigation history to work with. If you got to the page by opening a direct link (from email, from chat, ...), clicking on the "back" arrow will do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsnajdr has left some good suggestions that would be nice to address. As far as I can tell this is working well.
I've done some smoke testing and things work well.
- I'm not sure how to get a disconnected Atomic site so did not test any of those flows.
- Atomic sites are redirected away from
/settings/disconnect-site
. - Simple sites are redirected away from
/settings/disconnect-site
. - The disconnected Jetpack flows worked well in my testing.
Two experience notes:
- If a Jetpack site is properly connected, there's no indication of that in the disconnect site view. This is an edge case and may not be worth addressing in this iteration, but an indication when a site appears to be properly connected would be nice.
- The only support option seems to be a link to a documentation page. Even for sites with a page plan, an opportunity to contact Happiness seems to be missing.
Co-authored-by: Filipe Varela <keoshi@keoshi.com>
Thanks for all those changes, @delan ! One question that popped in my head when looking at this was if it would be possible to add tracking to the different options before merging this PR? |
Done in f37a7cc!
|
You're the best, @delan !! 🙌 |
Documentation on the deployment process: PCYsg-5XR-p2 Feel free to reach out to me when you're ready and I'll help with the deployment 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Hi, @delan! If it isn't already, could you please rebase this branch on latest Calypso master prior to merging? There were some fixes merged a couple days ago that got ci/wp-desktop
tests working again (after a long series of breakages). The tests should be passing now, and we're just trying to be pro-active about making sure tests are ✅ before merging new changes.
@nsakaimbo Thanks for the heads up! ci/wp-desktop is now green on this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that all issues were addressed. Thank you for working on this!
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3815855 Hi @delan, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Translation for this Pull Request has now been finished. |
Changes proposed
This pull request implements the new call-to-action and flow for the “site cannot be accessed” indicator on non-Atomic sites (as described in this internal post).
On such sites, the CTA changes to “I’d like to fix this now”, and it points to a new page that replaces the disconnect survey with some options that are more helpful for the situation. The indicator on Atomic sites and the disconnect button on the Manage Connection page remain unchanged.
There are some differences between these changes and the design, as well as some known problems, so I’ve added comments for them below. Let me know if these changes need any automated tests.
Testing instructions
1. Site indicator
2. Old page
3. New page
?type=down
to the end of your URLScreenshots