KMW / 7465 Implement Insufficient Permissions Error CTA on KMW Tiles #7554
Conversation
…-insufficient-permissions-error-cta-kmw-tiles.
…ss text based on whether error is one of insufficient permissions or not.
…d on it's presence.
…cenarios to all KMW stories.
|
Build files for a7fddbe are ready:
|
There was a problem hiding this comment.
Thanks, @10upsimon. I've noticed some discrepancies between the implementation and the expected behavior, explicitly concerning the Trouble getting access? and Get help links. I've provided specific comments on the relevant sections.
Please review those comments and make the necessary changes to align the implementation with the requirements.
| @@ -132,7 +140,7 @@ export default function ReportErrorActions( { moduleSlug, error } ) { | |||
| </Fragment> | |||
| ) : ( | |||
| <Link href={ errorTroubleshootingLinkURL } external> | |||
| { __( 'Get help', 'google-site-kit' ) } | |||
| { getHelpText || __( 'Get help', 'google-site-kit' ) } | |||
There was a problem hiding this comment.
The current implementation replaces the Get help link, which is not the intended behavior. According to the IB, the goal is to add a descriptive label to the existing Get help link for the insufficient permissions error. This should be done like how the Retry didn't work? text is displayed alongside its respective link for the retryable errors.
Specifically, the IB states:
- If the
getHelpTextprop is provided, render theGet helplink with the provided text, similar to howRetry didn’t work?andGet helpare rendered.
| { getHelpText || | ||
| __( | ||
| 'Get help', | ||
| 'google-site-kit' | ||
| ) } |
There was a problem hiding this comment.
We don't need to change this. Please refer to the comment below.
There was a problem hiding this comment.
We have to update all these new VRT images once the Trouble getting access? and Get help link is implemented.
Co-authored-by: Hussain Thajutheen <[email protected]>
Co-authored-by: Hussain Thajutheen <[email protected]>
Co-authored-by: Hussain Thajutheen <[email protected]>
…mw-tiles' of github.com:google/site-kit-wp into kmw/7465-implement-insufficient-permissions-error-cta-kmw-tiles.
|
Size Change: +961 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
@hussain-t this is ready for another round of review, thank you. I've also implemented a fix for tiles returning an array of error objects, as per this slack thread. This fix can be found in commit at ed538cf |
There was a problem hiding this comment.
Thanks, @10upsimon. Overall, the changes look good. However, a few minor improvements need to be taken care of, which I will address myself to avoid another iteration.
| error={ error } | ||
| getHelpText={ | ||
| hasInsufficientPermissionsError | ||
| ? __( 'Trouble getting access?', 'google-site-kit' ) |
There was a problem hiding this comment.
Since we're appending the HelpLink in ReportErrorActions, it shouldn't be translatable here. The whole string incluidng the HelpLink should be translated.
| <div> | ||
| { getHelpText ? ( | ||
| createInterpolateElement( | ||
| `${ getHelpText } <HelpLink />`, |
There was a problem hiding this comment.
The whole string should be translated here instead. Please refer to the previous comment.
|
|
||
| const [ accountID, propertyID, webDataStreamID ] = [ | ||
| '12345', | ||
| '34567', | ||
| '56789', | ||
| ]; | ||
|
|
||
| // Set accountID, propertyID and webDataStreamID values. | ||
| registry | ||
| .dispatch( MODULES_SEARCH_CONSOLE ) | ||
| .receiveGetSettings( { | ||
| propertyID: 'http://example.com/', | ||
| } ); | ||
| .dispatch( MODULES_ANALYTICS ) | ||
| .setAccountID( accountID ); | ||
| registry | ||
| .dispatch( MODULES_ANALYTICS_4 ) | ||
| .setPropertyID( propertyID ); | ||
| registry | ||
| .dispatch( MODULES_ANALYTICS_4 ) | ||
| .setWebDataStreamID( webDataStreamID ); |
There was a problem hiding this comment.
We don't need to mock the Analytics data since this tile's report depends on the Search Console module. We could set the SC property ID according to the getServiceEntityAccessURL selector. Even that's unnecessary as the selector doesn't return undefined. However, it's preferred.
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; |
There was a problem hiding this comment.
This not related to this ticket. However, it was imported with the internal dependencies. I have fixed it.
There was a problem hiding this comment.
Nice one, @10upsimon. LGTM 🏅
Note: I have slightly tweaked the QAB to test the actual error state since it only aimed to simulate the error using the Tweak extension.
There was a problem hiding this comment.
Hey @10upsimon, nice work here. I've sent it back with a couple of comments - one small but important, the other super trivial :)
assets/js/modules/analytics-4/components/widgets/EngagedTrafficSourceWidget.stories.js
Outdated
Show resolved
Hide resolved
techanvil
deleted the
kmw/7465-implement-insufficient-permissions-error-cta-kmw-tiles
branch
Summary
Addresses issue:
Relevant technical choices
<MetricTileError>component error to conditionally show and pass text based on whether error is one of insufficient permissions or not.getHelpTextprop and type, and updated logic to conditionally showgetHelpTextbased on it's presence.<MetricTileError>component styles to accommodate anchor tag based buttons as opposed to justbuttonelements.InsufficientPermissionsscenarios to all KMW stories and updated VRT references accordingly.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