-
Notifications
You must be signed in to change notification settings - Fork 204
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
Add BACK_TO_TOP event and fire when button is clicked #4953
Conversation
Co-authored-by: zack <[email protected]>
Co-authored-by: zack <[email protected]>
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.
This is a great start ✨
The scroll button is on all scrollable pages, including such content pages as https://openverse.org/about, so we should not send the event when we are not on a search or a collection page.
To check the page we are on, we can use useRoute
, and check if the route.name
is one of search__image
, search__audio
, search
, image__collection
, audio_collection
. Note that the route names are usually Symbol
s, so you would need to use .toString()
on them, and then also remove the language suffix such as __en
.
Also, this is not mentioned in the issue, but I think the params should be similar to the once sent by REACH_RESULT_END
:
openverse/frontend/src/types/analytics.ts
Line 116 in 749259e
REACH_RESULT_END: { |
Please let me know if anything is unclear. If you think this is too much work, we can remove the events from this PR, and just ship the fix to scrolling on the search pages.
Hi @obulat what exactly do you mean by this? Should they match them exactly, or include them in addition to the ones listed in the issue? |
It's the latter, they should contain
and
Here's the way we get those properties for openverse/frontend/src/components/VLoadMore.vue Lines 38 to 50 in 4992762
Plausible does not allow us to send |
A couple little things:
Let me know if you'd rather I use other values! |
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.
This works for the search pages, but needs to be updated for other pages.
I have several suggestions:
- I did not give the correct route names in the previous suggestions. They should be fixed.
- The search pages scroll the main page, but all others scroll the body. This means that the code for
scrollPixels
andmaxScroll
works correctly only on the search pages. - Since the scroll values are calculated after the page was scrolled to top, they are set to 0 (or -1, because of the
|| -1
condition). - It's better to get the search type from the
mediaStore._searchType
, which filters out the unsupported types.
Here's a patch with suggestions on how to fix these problems:
Subject: [PATCH] Fix search type, route names and scroll values
---
Index: frontend/nuxt.config.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/nuxt.config.ts b/frontend/nuxt.config.ts
--- a/frontend/nuxt.config.ts (revision 431ccc43aaa8f58cacf18170c8f5cbddde48cbbd)
+++ b/frontend/nuxt.config.ts (date 1727112312513)
@@ -42,7 +42,7 @@
},
plausible: {
ignoredHostnames: ["localhost", "staging.openverse.org"],
- logIgnoredEvents: true,
+ logIgnoredEvents: false,
apiHost: "http://localhost:50290",
domain: "localhost",
},
Index: frontend/src/components/VScrollButton.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/components/VScrollButton.vue b/frontend/src/components/VScrollButton.vue
--- a/frontend/src/components/VScrollButton.vue (revision 431ccc43aaa8f58cacf18170c8f5cbddde48cbbd)
+++ b/frontend/src/components/VScrollButton.vue (date 1727116542815)
@@ -24,45 +24,44 @@
defineEmits<{ tab: [KeyboardEvent] }>()
-const ANALYTICS_ROUTES = [
- "search__image",
- "search__audio",
- "search",
- "image__collection",
- "audio_collection",
-]
+const SEARCH_ROUTES = ["search-image", "search-audio", "search"]
+const ANALYTICS_ROUTES = [...SEARCH_ROUTES, "image-collection", "audio-collection"]
const hClass = computed(() =>
props.isFilterSidebarVisible ? positionWithSidebar : positionWithoutSidebar
)
const scrollToTop = () => {
+ const routeName = route.name?.toString().split("__")[0]
+
+ if (!routeName || !ANALYTICS_ROUTES.includes(routeName)) {
+ window.scrollTo({ top: 0, left: 0, behavior: "smooth" })
+ return
+ }
+
+ const isSearchRoute = routeName && SEARCH_ROUTES.includes(routeName)
+
const mainPage = document.getElementById("main-page")
- const element = mainPage || window
+
+ const scrollPixels = isSearchRoute ? mainPage?.scrollTop : window.scrollY
+ const maxScroll = isSearchRoute ? mainPage?.scrollHeight : document.body.scrollHeight
+
+ const element = isSearchRoute ? mainPage || window : window
element.scrollTo({ top: 0, left: 0, behavior: "smooth" })
- if (ANALYTICS_ROUTES.some((r) => route.name?.toString().startsWith(r))) {
const kind: ResultKind =
searchStore.strategy === "default" ? "search" : "collection"
const collectionType = (searchStore.collectionValue as Collection) ?? "null"
$sendCustomEvent("BACK_TO_TOP", {
- searchType:
- searchStore.searchType === "model-3d" ||
- searchStore.searchType === "video"
- ? "all"
- : searchStore.searchType,
+ searchType: mediaStore._searchType,
kind,
query: searchStore.searchTerm,
resultPage: mediaStore.currentPage,
- scrollPixels: mainPage?.scrollTop || -1,
- maxScroll:
- mainPage && "scrollTopMax" in mainPage
- ? (mainPage.scrollTopMax as number)
- : -1,
+ scrollPixels: scrollPixels ?? -1,
+ maxScroll: maxScroll ?? -1,
collectionType,
collectionValue: searchStore.collectionValue ?? "null",
})
- }
}
</script>
Based on the contributor urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with contributor urgency are expected to be reviewed within 3 weekday(s)2. @d3jawu, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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.
I've added the suggested patch and also extracted the common properties for search-related analytics events.
Thank you for your contribution, @d3jawu !
Fixes
Resolves #2276 by @fcoveram.
Fixes #4948 by @d3jawu.
Description
Adds a new analytics event,
BACK_TO_TOP
, and fires it when the "back to top" button in the bottom-right corner is clicked.Fixes the behavior of the "back to top" button, which had stopped working on the search page. Verified that the behavior still works on other pages.
Testing Instructions
BACK_TO_TOP
is logged.Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin