-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Metris: improve marketplace activity metrics to prepare next version #10495
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10495 /- ##
=======================================
Coverage 58.01% 58.01%
=======================================
Files 185 185
Lines 6431 6431
Branches 1400 1400
=======================================
Hits 3731 3731
Misses 2236 2236
Partials 464 464
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
Looks good to me as long as we update the strapi/tracker repo too for these events
const { error, isLoading, data } = useFetchPluginsFromMarketPlace(); | ||
|
||
if (isLoading || error) { | ||
return <LoadingIndicatorPage />; | ||
} | ||
|
||
const handleDownloadPlugin = async pluginId => { | ||
emitEvent('willInstallPlugin', { pluginId }); |
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.
For other events with use the property plugin for the name of the plugin we should probably make this consistent
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.
ok, did you mean { plugin: pluginId }
. I made that update but if I misunderstood just let me know 🙃
88462ff
to
716d3c7
Compare
@@ -53,6 56,12 @@ const LeftMenuLinkContent = ({ | |||
<LinkLabel>{labelId}</LinkLabel> | |||
); | |||
|
|||
const handleClick = () => { |
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.
Can you set it in a useEffect
hook directly in the <MarketPlacePage />
?
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.
ok yes I considered it but wasn't sure which approach was better. Thanks.
@@ -38,6 41,7 @@ const MarketPlacePage = () => { | |||
const response = await request('/admin/plugins/install', opts, overlayblockerParams); | |||
|
|||
if (response.ok) { | |||
emitEvent('didInstallPlugin', { plugin: pluginId }); |
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.
Installing a plugin takes quite some time I am not sure that this event will ever pop up but let's keep it.
const memoizedEmit = useCallback(() => { | ||
emitEvent('didGoToMarketplace'); | ||
}, [emitEvent]); | ||
|
||
useEffect(() => { | ||
memoizedEmit(); | ||
}, [memoizedEmit]); | ||
|
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.
@soupette Is this the correct way to do this? Originally I thought if I just added an empty dependency array to the useEffect it would only fire on first render, but eslint wanted the emitEvent
function in the dependency array.
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.
the best way to remove the emit from the dependency array is to use a ref:
const emitEventRef = useRef(emitEvent);
useEffect(() => {
emitEventRef.current('didGotToMarketplace');
}, [])
e94e7ee
to
3fc576c
Compare
3fc576c
to
bf233da
Compare
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.
LGTM!
What does it do?
adds send events when user visits marketplace or downloads a plugin