Add recommendations tracking #107

mocca102 · 2023-11-22T18:15:33Z

Track recommendation click
Track recommendation view

src/components/Autocomplete/SectionItemsList/SectionItemsList.tsx

src/hooks/useCioAutocomplete.ts

src/hooks/useDownShift.ts

src/hooks/useSections.ts

src/hooks/useRecommendationsObserver.ts

esezen

This is looking great! Thanks for making the changes.

I left some small comments, let me know what you think

Lets also get a review from the Professor @deomsj

src/utils.ts

src/hooks/useDownShift.ts

src/utils.ts

deomsj

nice work on this @mocca102 and @esezen!!!

I've left a number of ideas, questions, and suggestions as inline comments in this review. Looking forward to hearing your thoughts on these points and having another look at this together on a pairing call or async for another round of review.

I appreciate all of your great work on this & I'll follow your lead on how I can best support with next steps :)

src/hooks/useCioAutocomplete.ts

src/hooks/useDownShift.ts

deomsj · 2023-12-11T17:23:28Z

src/hooks/useRecommendationsObserver.ts

+    .map((section) => section.ref);
+
+  useEffect(() => {
+    const observer = new IntersectionObserver(


I'm confused. What do we mean by "intersection" here?

whoa... https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

this is very coooool :)

deomsj · 2023-12-11T17:33:17Z

src/hooks/useRecommendationsObserver.ts

+import ConstructorIO from '@constructor-io/constructorio-client-javascript';
+import { Section } from '../types';
+
+function useRecommendationsObserver(


can we add some brief comment above this function definition describing the scope of responsibility for this hook? We are using some very cool patterns in here that will not be obvious to many developers. I'd love to see explicit documentation for what this hook is doing for us and what needs to be configured elsewhere in the lib code or in a lib consumer's src code in order for this to behave as it is intended

Let me know what you think 55c168e#diff-54d8f2b4755e1b9a3357013b05b24a47635a9fbae6dc1ab0116aa0dcaf69aa05R7

YES. Well done sir. I found these explanations to be very, very helpful :)

deomsj · 2023-12-11T17:37:46Z

src/hooks/useSections.ts

+  useEffect(() => {
+    setActiveSections(zeroStateActiveSections ? zeroStateSections : sections);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [zeroStateActiveSections]);


I may be missing something, but this looks like derived state at this point.
Can we simplify?

all of this:

// Define All Sections const [activeSections, setActiveSections] = useState<UserDefinedSection[]>( zeroStateActiveSections ? zeroStateSections : sections ); useEffect(() => { setActiveSections(zeroStateActiveSections ? zeroStateSections : sections); // eslint-disable-next-line react-hooks/exhaustive-deps }, [zeroStateActiveSections]);

to just, this:
this:

const activeSections = zeroStateActiveSections ? zeroStateSections : sections;

I thought I wanted to make it reactive to only zeroStateActiveSections so it updates the state only if zeroStateActiveSections changes. But whenever zeroStateActiveSections, zeroStateSections and sections also change and they're passed as props to the hook so the whole hook rerun every time any way.

TLDR; I thought this was an optimization but I don't think it is anymore

55c168e#diff-c72a6a39c2792562569f2b7e56003020f1eefbc541ba6221750d044635f0d1b1R20

deomsj · 2023-12-11T17:44:06Z

src/hooks/useSections.ts

+    );
+  }, [autocompleteResults, recommendationsResults, activeSections]);

+  // Reset sections


why do we need this effect? I'm wondering why we need to manage activeSections as its own state here at all instead of deriving it from other state passed in to this hook...

deomsj · 2023-12-11T17:48:01Z

src/utils.ts

+
+      if (sectionConfig.type === 'recommendations') {
+        // If user provided a ref in config, use it. Otherwise, use the ref from our refs array
+        section.ref = sectionConfig.ref || sectionsRefs.current[index];


when would this value arrive to this point via sectionConfig.ref?

when would this value arrive to this point via sectionsRefs.current[index]

can we add comments or use some precisely named consts here to make this explicit to anyone referencing or building on top of this code?

Let me know if this clears things up 55c168e#diff-39b2554fd18da165b59a6351b1aafff3714e2a80c1435f2de9706355b4d32351R173

src/utils.ts

deomsj

Nicely done @mocca102!

I haven't tried breaking functionality locally yet, but I'm lovin the changes made here from a code clarity / maintainability perspective.

I left a couple more questions & ideas. Holla for pairing sesh or for another review as you like :)

deomsj · 2023-12-12T14:29:00Z

src/hooks/useRecommendationsObserver.ts

+ * Custom hook that observes the visibility of recommendation sections and calls trackRecommendationView event.
+ * This is done by using the IntersectionObserver API to observe the visibility of each recommendation section.
+ * That is done by passing the ref of each recommendation section to the IntersectionObserver.
+ * The refs are either passed by the user in section config or generated by the library by default.


vocab alignment:

"user" - end user who's browser this production code is rendered on

"lib consumer" - developer / team who is consuming this library and building on top of it

deomsj · 2023-12-12T14:31:25Z

src/utils.ts

+        data: sectionData,
+      };
+
+      // If user provided a ref in `sectionConfig`, use it. Otherwise, use the ref from our library generated refs array


Four questions:

how will the lib consumer know that they can pass these refs?

how will lib consumer know how to pass this ref?

what happens if they don't pass this ref and are using the lib's component interface?

what happens if they don't pass this ref and are using the lib's hook interface?

It's a prop on type SectionConfiguration https://github.com/Constructor-io/constructorio-ui-autocomplete/pull/107/files/55c168e54f3bc1568827cba615592e193f2fc3c1#diff-c54113cf61ec99691748a3890bfbeb00e10efb3f0a76f03a0fd9ec49072e410aR83

same as 1 but we can add more documentation explaining that

and 4. There's no change between hooks and component interface as long as they use getSectionProps

We need to add docs for getSectionProps

This is a great callout. This would have made things more clear if added from the beginning. Added here
06ced4f#diff-8fa4b52909f895e8cda060d2035234e0a42ca2c7d3f8f8de1b35a056537bf199R40 @deomsj

deomsj

Findings from dev build on my machine:

all storybook tests are passing!
all rec view and rec click events look great! (screenshot below for more detail)
tried a handfull of stories without recs and both UX and network traffic continue to behave as expected!

Excellent work on this, @mocca102!!!