feat: add scolled_to trigger #244
Conversation
Add 'scrolled_to' trigger and publish scroll events via
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| wrapHistoryMethods(); | ||
| }; | ||
|
|
||
| private setupScrolledToPublisher = () => { |
There was a problem hiding this comment.
setupScrolledToPublisher adds a new scroll listener every time initSubscriptions() runs, but never removes the old one. This can leak and double-fire in visual editor mode where initSubscriptions() is called repeatedly. Consider removing any previous listener before attaching a new one (or keep a single shared handler).
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
|
📝 Documentation updates detected! Updated existing suggestion: Document new page trigger types for Web Experiment |
| wrapHistoryMethods(); | ||
| }; | ||
|
|
||
| private setupScrolledToPublisher = () => { |
There was a problem hiding this comment.
scrolled_to publishes on every scroll once a target is met, unlike element_visible which de-dupes after first fire. Consider tracking which percentages/elements have already fired and only publish when a new target is first reached to avoid repeatedly triggering applyVariants() during scrolling.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| for (const pages of Object.values(this.pageObjects)) { | ||
| for (const page of Object.values(pages)) { | ||
| if (page.trigger_type === 'scrolled_to') { |
There was a problem hiding this comment.
nit: I see a repeated pattern for each trigger types. Consider refactoring.
|
|
||
| this.globalScope.addEventListener( | ||
| 'scroll', | ||
| () => handleScroll(minPercentage, elementTargets), |
There was a problem hiding this comment.
Consider throttling (e.g. requestAnimationFrame) since the scroll event can fire a lot. Each fires querySelector + getBoundingClientRect for every element target, forcing a reflow.
| const scrollPosition = this.globalScope.scrollY; | ||
|
|
||
| if ( | ||
| scrollPosition + this.globalScope.innerHeight >= | ||
| elementPosition + scrollPosition + offsetPx | ||
| ) { |
There was a problem hiding this comment.
scrollPosition would be canceled out. Should remove this.
| const elementAndOffset = new Set<string>(); | ||
|
|
||
| // Check which elements are in range | ||
| for (const { selector, offsetPx } of elements) { |
There was a problem hiding this comment.
nit: WDYT about using IntersectionObserver for scrolled to element trigger type?
| const elementAndOffset = scrollPayload.elementAndOffset; | ||
|
|
||
| if (triggerValue.mode === 'percent') { | ||
| return currentScrollPercentage >= triggerValue.percentage; |
There was a problem hiding this comment.
The “scrolled to” condition should not be re-evaluated when the user scrolls back up. Once it’s exposed, it should remain active until the exit condition occurs—in this case, a URL change (the default exit condition).
| // Publish if minimum percentage threshold is met or any element is in range | ||
| const shouldPublish = | ||
| (minPercent !== undefined && scrollPercentage >= minPercent) || | ||
| elementAndOffset.size > 0; | ||
|
|
||
| if (shouldPublish) { | ||
| this.messageBus.publish('scrolled_to', { | ||
| scrollPercentage, | ||
| elementAndOffset, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Given that other trigger types evaluate in the subscription, I'd suggest moving this logic. Also you won't have to deal with this if you change the element scroll with IntersectionObserver.
Summary
Support
scrolled_topage trigger, which fires when:percentagemode: user scrolls down the specified minimum percent of the pageelementmode: user scrolls down to an element (offset by optionaloffsetPx)Checklist