Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

fix(recommendations): prepping recommendations to fire impression events for analytics #58

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

anthony-liddle
Copy link

Goal

Prep recommendations to send analytics events

To Do:

  • Break recommendation into its own template
  • Add intersection observer function

Implementation Decisions

My work with the intersection observer was based on the @vueuse/core package.

@anthony-liddle anthony-liddle requested a review from a team as a code owner October 10, 2023 20:16
const target = ref<HTMLDivElement>()
const { isActive } = useIntersectionObserver(target, checkIntersection)

function checkIntersection([{ isIntersecting }]: [{ isIntersecting: boolean }]): void {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line hurts to dissect at first but got better afterwards 😃

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it, too. I could probably add some line breaks to make it a little easier to grok.

</div>
</NuxtLink>
</template>
<RecommendationArticle v-for="item in recommendations" :key="item.id" :item="item" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the key changing from item.tileId intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! This matches up with the new API.

It should have been done before, but since we weren't using this value for anything it was missed.

Copy link

@jpezninjo jpezninjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link

@wtfluckey wtfluckey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!!

One question and something that could be addressed in a separate PR - the ticket mentions An impression should fire when a % of it is visible on the screen. Make this % adjustable for now.. I confirmed with Daniel that this can be a simple config variable and not something that like an admin or someone needs access to. We could do that by adding a threshold, right?

@anthony-liddle anthony-liddle merged commit 7312937 into main Oct 11, 2023
3 checks passed
@anthony-liddle anthony-liddle deleted the fix/recommendations-instersection-observer branch October 11, 2023 16:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants