Custom dimension tile wrapper #7680
Conversation
|
Build files for 6553776 are ready:
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Looks great!
A few notes on the translation strings here, but they're minor.
I did encounter an issue when following the QA Brief:
Upon saving your selection, you should be directed to the OAuth flow. Cancel the OAuth flow to return to the SK dashboard.
I wasn't redirected, instead I was immediately shown this screen:
Verify that clicking on "Update" makes the tiles go on a loading state
I don't see the loading state if I haven't granted OAuth access and am being redirected to the OAuth flow:
CleanShot.2023-10-13.at.15.54.25.mp4
Is that expected?
Even after I've granted access, I don't see a network request when clicking "Update":
CleanShot.2023-10-13.at.15.55.36.mp4
| { createInterpolateElement( | ||
| __( | ||
| 'Permissions updated? <RetryLink />', | ||
| 'google-site-kit' | ||
| ), | ||
| { | ||
| RetryLink: ( | ||
| <Link | ||
| onClick={ | ||
| handleCreateCustomDimensions | ||
| } | ||
| > | ||
| { __( | ||
| 'Retry', | ||
| 'google-site-kit' | ||
| ) } | ||
| </Link> | ||
| ), | ||
| } | ||
| ) } |
There was a problem hiding this comment.
No need to have the separate translation string here, this should work:
| { createInterpolateElement( | |
| __( | |
| 'Permissions updated? <RetryLink />', | |
| 'google-site-kit' | |
| ), | |
| { | |
| RetryLink: ( | |
| <Link | |
| onClick={ | |
| handleCreateCustomDimensions | |
| } | |
| > | |
| { __( | |
| 'Retry', | |
| 'google-site-kit' | |
| ) } | |
| </Link> | |
| ), | |
| } | |
| ) } | |
| { createInterpolateElement( | |
| __( | |
| 'Permissions updated? <a>Retry</a>', | |
| 'google-site-kit' | |
| ), | |
| { | |
| a: ( | |
| <Link | |
| onClick={ | |
| handleCreateCustomDimensions | |
| } | |
| /> | |
| ), | |
| } | |
| ) } |
There was a problem hiding this comment.
(I realise an argument to separating out the small translation strings is that things like __( 'Retry' ) and __( 'Learn more' ) are already translated, but keeping as much context in one string is usually best, and since the main string needs to be translated anyway, I think it's fine 😄 )
There was a problem hiding this comment.
Updated, thank you!
| { createInterpolateElement( | ||
| __( | ||
| 'You’ll need to contact your administrator. <LearnMoreLink />', | ||
| 'google-site-kit' | ||
| ), | ||
| { | ||
| LearnMoreLink: ( | ||
| <Link href={ helpLink } external> | ||
| { __( | ||
| 'Learn more', | ||
| 'google-site-kit' | ||
| ) } | ||
| </Link> | ||
| ), | ||
| } | ||
| ) } |
There was a problem hiding this comment.
Same here:
| { createInterpolateElement( | |
| __( | |
| 'You’ll need to contact your administrator. <LearnMoreLink />', | |
| 'google-site-kit' | |
| ), | |
| { | |
| LearnMoreLink: ( | |
| <Link href={ helpLink } external> | |
| { __( | |
| 'Learn more', | |
| 'google-site-kit' | |
| ) } | |
| </Link> | |
| ), | |
| } | |
| ) } | |
| { createInterpolateElement( | |
| __( | |
| 'You’ll need to contact your administrator. <a>Learn more</a>', | |
| 'google-site-kit' | |
| ), | |
| { | |
| a: ( | |
| <Link href={ helpLink } external /> | |
| ), | |
| } | |
| ) } |
There was a problem hiding this comment.
Updated, thank you!
| { createInterpolateElement( | ||
| __( | ||
| 'Retry didn’t work? <GetHelpLink />', | ||
| 'google-site-kit' | ||
| ), | ||
| { | ||
| GetHelpLink: ( | ||
| <Link href={ helpLink } external> | ||
| { __( | ||
| 'Learn more', | ||
| 'google-site-kit' | ||
| ) } | ||
| </Link> | ||
| ), | ||
| } | ||
| ) } |
There was a problem hiding this comment.
Same here:
| { createInterpolateElement( | |
| __( | |
| 'Retry didn’t work? <GetHelpLink />', | |
| 'google-site-kit' | |
| ), | |
| { | |
| GetHelpLink: ( | |
| <Link href={ helpLink } external> | |
| { __( | |
| 'Learn more', | |
| 'google-site-kit' | |
| ) } | |
| </Link> | |
| ), | |
| } | |
| ) } | |
| { createInterpolateElement( | |
| __( | |
| 'Retry didn’t work? <a>Learn more</a>', | |
| 'google-site-kit' | |
| ), | |
| { | |
| a: ( | |
| <Link href={ helpLink } external /> | |
| ), | |
| } | |
| ) } |
There was a problem hiding this comment.
Updated, thank you!
There was a problem hiding this comment.
Thanks @nfmohit! This is a big one and looking good. From my POV this looks quite close. There are some things that could be improved but we need to balance that with unblocking other important issues that are waiting for this to land, so let's focus on the most important things at this point and revisit any secondary points in a follow-up. Nice work!
| @@ -95,6 +103,7 @@ CTA.propTypes = { | |||
| error: PropTypes.bool, | |||
| onClick: PropTypes.func, | |||
| children: PropTypes.node, | |||
| headerContent: PropTypes.node, | |||
There was a problem hiding this comment.
Minor detail, but good to co-locate related prop definitions if we're not sorting the list another way. i.e. putting header* props together.
There was a problem hiding this comment.
Updated, thank you!
| registry | ||
| .select( MODULES_ANALYTICS_4 ) | ||
| .getCreateCustomDimensionError( | ||
| 'googlesitekit_post_date' | ||
| ) |
There was a problem hiding this comment.
We should avoid using selectors for making assertions about actions. Actions are only concerned with changing state or producing side effects (like a request via a control). In this case, the assertion should be about the state. This way the action tests also cover the relevant portion of the reducer and won't break if the selector is implemented incorrectly.
There was a problem hiding this comment.
Understood, thank you @aaemnnosttv. I only used the selector here because it was a little inconvenient to access the error in the store because we need to generate the error key using the custom dimension options, which we don't have access to as it is not exported from custom-dimensions.js.
However, I've worked around that by adding a local copy of the custom dimension options in the test case and generating the error copy there.
Let me know what you think, thanks!
| const { dimensions, infoTooltip, reportOptions, title } = options; | ||
|
|
||
| return ( WrappedComponent ) => { | ||
| const WithCustomDimensionsComponent = ( props ) => { |
There was a problem hiding this comment.
This component is quite large, it would be great to refactor it to be composed of a few inner components to reduce the overall complexity. We probably don't have time to do it now so this would be nice to do as a future follow-up.
There was a problem hiding this comment.
I agree. I'll open a follow-up issue as soon as the PR is approved otherwise, thanks!
| title: definedTitle, | ||
| } = KEY_METRICS_WIDGETS[ widgetSlug ] || {}; | ||
|
|
||
| const newsKeyMetricsEnabled = useFeature( 'newsKeyMetrics' ); |
There was a problem hiding this comment.
This guard is unnecessary because the only widgets that use it will be guarded by it, as well as KMW itself. Aside from that, if we did want such a guard, this is too late to use it because it won't stop all of the effects and other hooks from running.
Not every single thing used by a feature needs to include a guard for it 😄
|
|
||
| it( 'renders appropriate error if required custom dimensions are not available', () => { | ||
| const { container } = render( <WithCustomDimensionsComponent />, { | ||
| setupRegistry: ( registry ) => { |
There was a problem hiding this comment.
I find it cleaner to initialize the test registry in a beforeEach and then pass it to in the render options rather than doing the other way around (we're too flexible! 😄)
This way we can't apply common setup (the way beforeEach is intended to be used).
In this case it might only be
provideUserAuthentication( registry );
provideUserCapabilities( registry );but that does help keep each test a bit more concise.
There was a problem hiding this comment.
Updated, thank you!
Thank you for the kind words and review, @aaemnnosttv. I've addressed the feedback, thanks! |
assets/js/modules/analytics-4/components/widgets/PopularAuthorsWidget.stories.js
Outdated
Show resolved
Hide resolved
| isSyncingAvailableCustomDimensions( state ) { | ||
| return !! state?.isSyncingAvailableCustomDimensions; | ||
| }, |
There was a problem hiding this comment.
How is isSyncingAvailableCustomDimensions different from isFetchingSyncAvailableCustomDimensions? These are completely similar selectors with exactly the same logic. If we worry that isFetching may cause race conditions, then isSyncing has the same problem. I think that if we truly want to be safe here, we should use debounce the sync action and call it in the create dimensions action.
cc @aaemnnosttv
| * @param {Object} error The error object. | ||
| * @param {string} customDimension Custom dimension to set creation error for. | ||
| */ | ||
| *receiveCreateCustomDimensionError( error, customDimension ) { |
There was a problem hiding this comment.
Let's make this action to be a utility function then and have the same approach as we use for provide* helpers. So we will have something like this: provideCustomDimensionsError( registry, { customDimension, error } ).
Without an action like this, we'll have to export
possibleCustomDimensions(as a constant, for example)
Fine with me.
…mDimensionError` test utility.
There was a problem hiding this comment.
Looks good to me, but I noticed all E2E tests and the JS tests were failing. I re-ran them a few times and it seems something's actually off with them, can you look before we merge @eugene-manuilov?
Summary
Addresses issue:
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