Add a get help link to mismatched url errors. #5636
Conversation
maciejost
force-pushed
the
enhancement/##5481-add-get-help
branch
from
b0478b7
to
237d421
Compare
|
Size Change: +2.2 kB (0%) Total Size: 6.23 MB
ℹ️ View Unchanged
|
| @@ -1113,11 +1113,13 @@ private function get_reconnect_after_url_mismatch_notice() { | |||
| $current_url = $this->context->get_canonical_home_url(); | |||
| $content = '<p>' . sprintf( | |||
| /* translators: 1: Plugin name. 2: Message. 3: Proxy setup URL. 4: Reconnect string. */ | |||
| __( '%1$s: %2$s <a href="%3$s">%4$s</a>', 'google-site-kit' ), | |||
| __( '%1$s: %2$s <a href="%3$s">%4$s</a>. <a targe="_blank" href="%5$s">%6$s</a>', 'google-site-kit' ), | |||
There was a problem hiding this comment.
Looks like a typo here.
| __( '%1$s: %2$s <a href="%3$s">%4$s</a>. <a targe="_blank" href="%5$s">%6$s</a>', 'google-site-kit' ), | |
| __( '%1$s: %2$s <a href="%3$s">%4$s</a>. <a target="_blank" href="%5$s">%6$s</a>', 'google-site-kit' ), |
| @@ -1113,11 +1113,13 @@ private function get_reconnect_after_url_mismatch_notice() { | |||
| $current_url = $this->context->get_canonical_home_url(); | |||
| $content = '<p>' . sprintf( | |||
| /* translators: 1: Plugin name. 2: Message. 3: Proxy setup URL. 4: Reconnect string. */ | |||
There was a problem hiding this comment.
This comment should have the two new properties being added to the gettext string here, so translators know what each variable is to help translation context.
| esc_html__( 'Reconnect', 'google-site-kit' ) | ||
| esc_html__( 'Reconnect', 'google-site-kit' ), | ||
| esc_url( $this->get_proxy_support_link_url() . '/?doc=url-has-changed' ), | ||
| esc_html__( 'Get Help', 'google-site-kit' ) |
There was a problem hiding this comment.
| esc_html__( 'Get Help', 'google-site-kit' ) | |
| esc_html__( 'Get help', 'google-site-kit' ) |
We can lowercase this one too.
| external | ||
| > | ||
| { __( | ||
| 'Get Help', |
There was a problem hiding this comment.
| 'Get Help', | |
| 'Get help', |
We already use Get help as a translation string elsewhere in the plugin, and it's more grammatically correct, so let's lowercase the "h" 😄
| { getHelp && ( | ||
| <Link | ||
| href={ getHelp } | ||
| external | ||
| > |
There was a problem hiding this comment.
I don't quite understand the logic flow here… changedURLHelpLink is assigned to this variable only when it's truthy, and then getHelp is checked for truthiness in the render…
But I think checking changedURLHelpLink directly would result in the same logic without making the new variable, right?
{ changedURLHelpLink && (
<Link
href={ changedURLHelpLink }
external
>EDIT: Oh, I see, getHelp is only set when 'revoked' === getQueryArg( location.href, 'googlesitekit_context' ).
In that case, might be better to assign a variable to the above check, eg.:
const authenticationRevoked = 'revoked' === getQueryArg( location.href, 'googlesitekit_context' );And then use that as the check to display the help text, eg:
| { getHelp && ( | |
| <Link | |
| href={ getHelp } | |
| external | |
| > | |
| { authenticationRevoked && ( | |
| <Link | |
| href={ changedURLHelpLink } | |
| external | |
| > |
There was a problem hiding this comment.
It's the other way around, getHelp is assigned only if 'revoked' !== getQueryArg( location.href and disconnectedReason === connected_url_mismatch.
Should I extract that conditional out to its own variable, or should I just let it stay in the same statement with the titles and descriptions?
There was a problem hiding this comment.
Oh, I see, the GitHub diff made it a bit hard to catch that.
I think your solution works, but let's initialise the getHelp variable as null instead of empty string… and maybe call it getHelpURL. Just for a bit of extra clarity in variable naming.
| { getHelp && ( | ||
| <Link | ||
| href={ getHelp } | ||
| external | ||
| > |
There was a problem hiding this comment.
Oh, I see, the GitHub diff made it a bit hard to catch that.
I think your solution works, but let's initialise the getHelp variable as null instead of empty string… and maybe call it getHelpURL. Just for a bit of extra clarity in variable naming.
| if ( changedURLHelpLink ) { | ||
| getHelp = changedURLHelpLink; | ||
| } |
There was a problem hiding this comment.
This logic probably isn't needed, because if changedURLHelpLink is false-y (eg is undefined because the resolver is still loading) it still won't show anything as getHelp will still be false-y. I get why this is here but it's probably not needed actually. 🙂
…ite-kit-wp into enhancement/##5481-add-get-help.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist