Add shouldPromptGA4DashboardView selector and SwitchGA4DashboardViewNotification component.
#6752
Conversation
…iew` selector. The AC only calls for the property age, not the data state.
|
Size Change: +714 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
…PromptGA4DashboardView`.
…d of duplicating logic.
There was a problem hiding this comment.
Hi @kuasha420, this looks good, I've left a few comments but they are all of a pretty trivial nature. Please see what you think.
|
|
||
| const { setDashboardView, saveSettings } = useDispatch( MODULES_ANALYTICS ); | ||
|
|
||
| const handleCTAClick = useCallback( async () => { |
There was a problem hiding this comment.
As discussed in this comment, we keep on using useCallback() in places where it doesn't actually give us any performance improvement as the components the resulting callbacks are passed to are not memoized, which means rather than a performance improvement, useCallback() is actually a performance hit for no gain.
I think we should stop this cargo-cult-esque usage of useCallback() and only apply it where it actually makes sense to do so. It's not serving any purpose in this instance, as neither SpinnerButton or the Button it's passed to are memoized, so it can be removed here.
The article linked to in the comment is worth a read for a refresh on this stuff.
| const handleCTAClick = useCallback( async () => { | |
| const handleCTAClick = async () => { |
There was a problem hiding this comment.
Very interesting, I have removed the useCallback from this one for the reasons you've mentioned.
There's a lot of instances of this in our codebase and it's easy to just follow prior art, so do you think it's a good idea to reassess the use of useCallback and useMemo in our codebase and remove the unnecessary ones? If you think so, feel free to create an issue for it.
Cheers
There was a problem hiding this comment.
Thanks @kuasha420! I agree, it would be good to review our usage of these hooks and remove the unnecessary ones. I've created #6761 to track this.
assets/js/components/notifications/SwitchGA4DashboardViewNotification.js
Outdated
Show resolved
Hide resolved
| ctaComponent={ | ||
| <SpinnerButton | ||
| className="googlesitekit-notification__cta" | ||
| onClick={ handleCTAClick } | ||
| isSaving={ isSaving } | ||
| disabled={ isSaving } | ||
| > | ||
| { __( 'Update dashboard', 'google-site-kit' ) } | ||
| </SpinnerButton> | ||
| } |
There was a problem hiding this comment.
I realise this is following the IB, but considering how BannerNotification uses SpinnerButton, I think we could potentially implement this without ctaComponent, although we'd need to pass something to ctaLink in order for it to display - we passed a lone '# in other cases.
| ctaComponent={ | |
| <SpinnerButton | |
| className="googlesitekit-notification__cta" | |
| onClick={ handleCTAClick } | |
| isSaving={ isSaving } | |
| disabled={ isSaving } | |
| > | |
| { __( 'Update dashboard', 'google-site-kit' ) } | |
| </SpinnerButton> | |
| } | |
| ctaLink="#" | |
| ctaLabel={ __( 'Update dashboard', 'google-site-kit' ) } | |
| onCTAClick={ handleCTAClick } |
What do you think?
There was a problem hiding this comment.
I wasn't sure if putting a link # will trigger the onClick but from my testing it does, so easy win here unless this causes any accessibility issues.
On a side note, I think we could do with a refactoring of the BannerNotification at some point so this ctaLink is not required when there's a label and click handler available in a future issue.
Cheers.
There was a problem hiding this comment.
Agreed, although, I'm hoping we can take a more wholesale approach to BannerNotification improvements via #6730, so am a little hesitant to add an issue for this detail. If we don't get round to this redesign any time soon then it does feel like a detail that we should address in the current implementation.
assets/js/components/notifications/SwitchGA4DashboardViewNotification.stories.js
Outdated
Show resolved
Hide resolved
| component: SwitchGA4DashboardViewNotification, | ||
| decorators: [ | ||
| ( Story ) => { | ||
| enabledFeatures.add( 'ga4Reporting' ); |
There was a problem hiding this comment.
Features can be passed on a per-story basis using parameters. If all the stories for this component should have this feature, we could maybe set it at the Template level. Otherwise this is probably okay, but isn't what we normally do.
There was a problem hiding this comment.
The FeatureProvider or components incorporating that (ie. WithTestRegistry) didn't work here because that only provides the feature in react component context, but we're using isFeatureEnabled in the datastore code that requires the "global" enabledFeatures set to have the feature flag, that's why I had to take this approach. Probably could've parameterized this, however. 👍
| .dispatch( MODULES_ANALYTICS_4 ) | ||
| .receiveIsGatheringData( false ); | ||
| return ( | ||
| <WithTestRegistry registry={ registry }> |
There was a problem hiding this comment.
WithTestRegistry shouldn't be used unless we really need it for some reason. We should be using WithRegistrySetup instead.
There was a problem hiding this comment.
I think I initially used WithTestRegistry as it has a features prob, however that approach didn't work for the reason I mentioned in the other comment, I didn't bother refactoring it to use WithRegistrySetup afterwards because things seemed to work and probably laziness. 😅 Thanks for letting me know that WithRegistrySetup is generally preferred! 💪
Summary
Addresses issue:
Relevant technical choices
Implementation slightly derives from IB - instead of duplicating
isGatheringDatalogic and adapt it to be 3 days, we changed theisGatheringDatato to return true for properties less 3 days old.The banner is also only rendered if the Analytics module is connected, as discussed with @nfmohit.
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