Analytics event: CHANGE_CONTENT_TYPE
#1151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zackkrida
changed the title
CHANGE_CONTENT_TYPE analytics event
|
Size Change: -17.3 kB (-2%) Total Size: 845 kB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/1151 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
|
@zackkrida Is this PR blocked? |
|
@WordPress/openverse-frontend apologies but I am going to need to hand this off to someone else. We can assign in the team meeting tomorrow 👍 |
obulat
force-pushed
the
add/events-change-content-type
branch
from
1fed6b4 to
a2ea4e9
Compare
obulat
added
🟨 priority: medium
zackkrida
changed the title
CHANGE_CONTENT_TYPE analytics eventCHANGE_CONTENT_TYPE analytics event
obulat
force-pushed
the
add/events-change-content-type
branch
from
247593d to
e62dd0b
Compare
obulat
changed the title
CHANGE_CONTENT_TYPE analytics eventCHANGE_CONTENT_TYPE
|
Thanks for working on this, @obulat 😄 |
obulat
force-pushed
the
add/events-change-content-type
branch
2 times, most recently
from
7c2b8de to
17f38f7
Compare
dhruvkb
approved these changes
May 3, 2023
| @@ -64,6 +66,7 @@ export const useAnalytics = () => { | |||
| name: T, | |||
| payload: Events[T] | |||
| ) => { | |||
| log(`Analytics event: ${name}`, payload) | |||
| */ | ||
| export const useComponentName = () => { | ||
| const vm = getCurrentInstance() | ||
| return vm?.proxy.$options.name |
There was a problem hiding this comment.
I don't think name is a required property so having a fallback in case it's undefined might be a good idea. A lint check to ensure all our components have names would be a better idea but that might be more work.
There was a problem hiding this comment.
Another thought: should useComponentName raise an error if it's used in a context where no component name exists? That would keep the return value stable string not undefined | string, but would be much stricter than this implementation.
Falling back to "Unknown" without it being intentional, especially if this is used to disambiguate events, doesn't seem like a good long-term practice.
There was a problem hiding this comment.
I would like to have a stricter implementation, but when we test the use-search-type in unit tests, there is no component, so this will throw errors. Maybe we could create a new issue to see if we have many "Unknown" components in analytics reports, and if we do - fix it?
| analytics.sendCustomEvent("CHANGE_CONTENT_TYPE", { | ||
| previous: previousSearchType.value, | ||
| next: searchType, | ||
| component: componentName ?? "Unknown", |
There was a problem hiding this comment.
?? "Unknown" should be moved to the composable itself.
sarayourfriend
previously requested changes
May 4, 2023
There was a problem hiding this comment.
This works fine locally, but as with the other event PRs, I have some questions about the payload data, what we're trying to identify with them, and whether we need to tighten the requirements for the useComponentName composable or choose a different approach altogether for disambiguating this particular event.
Comment on lines
+116
to
+117
| /** The name of the Vue component used to switch content types. */ | ||
| component: string |
There was a problem hiding this comment.
I kind of wonder about using the Vue component name here rather than more stable names. We don't change component names often but the header dropdown I think has had its components changed during the redesign. In cases like that, the name of the component might change, even though the interaction area stayed the same 🤔
This also makes me wonder whether we should send the feature flag state in the payload for all events? If we have a feature flag that toggles a particular bit of UI or a feature on or off and it suddenly changes how a custom event trends, it would be nice to see if the feature flag preferences were changed for those events.
It isn't an issue now because I don't think we have any feature flags that would qualify for that, but something to consider for the future, especially if we use things like component names which are not necessarily stable, even if the UI area they're inside of remains stable.
On the other hand, using the component name might be a good way to tell the same cases as those feature flags, but probably contingent on the implementation.
No real suggestion for a change here, just some musings about how to find stable identifiers and thing carefully about exactly what each payload item is meant to help us do. If we ask the question, why do we want to know the component name for this event, does the answer lead us to think that a different approach to this payload field would be appropriate?
| */ | ||
| export const useComponentName = () => { | ||
| const vm = getCurrentInstance() | ||
| return vm?.proxy.$options.name |
There was a problem hiding this comment.
Another thought: should useComponentName raise an error if it's used in a context where no component name exists? That would keep the return value stable string not undefined | string, but would be much stricter than this implementation.
Falling back to "Unknown" without it being intentional, especially if this is used to disambiguate events, doesn't seem like a good long-term practice.
AetherUnbound
approved these changes
May 4, 2023
obulat
force-pushed
the
add/events-change-content-type
branch
from
17f38f7 to
5724117
Compare
|
Size Change: -17.5 kB (-2%) Total Size: 840 kB
ℹ️ View Unchanged
|
zackkrida
added
🟧 priority: high
|
@obulat this has two approving reviews despite some dangling feedback from Sara. Could you review and merge, making any necessary follow-up issues? |
obulat
force-pushed
the
add/events-change-content-type
branch
from
5724117 to
2960306
Compare
obulat
dismissed
sarayourfriend’s stale review
I'll open a new issue for the points raised by Sara
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1071 by @dhruvkb
Description
useComponentNamecomposableCHANGE_CONTENT_TYPEevent and fires it on use of theVSearchTypeandVContentLinkcomponentssendCustomEventmethodTODO
VLinkclick event situationTesting Instructions
Checklist
Update index.md).main) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin