-
Notifications
You must be signed in to change notification settings - Fork 12
feat: add scolled_to trigger #244
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
base: web/page-triggers-1
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import { | |
| ElementAppearedPayload, | ||
| ManualTriggerPayload, | ||
| MessageType, | ||
| ScrolledToPayload, | ||
| } from './message-bus'; | ||
| import { DebouncedMutationManager } from './mutation-manager'; | ||
| import { | ||
|
|
@@ -15,6 +16,7 @@ import { | |
| ManualTriggerValue, | ||
| PageObject, | ||
| PageObjects, | ||
| ScrolledToTriggerValue, | ||
| } from './types'; | ||
|
|
||
| const evaluationEngine = new EvaluationEngine(); | ||
|
|
@@ -66,6 +68,7 @@ export class SubscriptionManager { | |
| } | ||
| this.setupMutationObserverPublisher(); | ||
| this.setupVisibilityPublisher(); | ||
| this.setupScrolledToPublisher(); | ||
| this.setupPageObjectSubscriptions(); | ||
| this.setupUrlChangeReset(); | ||
| // Initial check for elements that already exist | ||
|
|
@@ -413,6 +416,106 @@ export class SubscriptionManager { | |
| wrapHistoryMethods(); | ||
| }; | ||
|
|
||
| private setupScrolledToPublisher = () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // Collect static list of scroll targets from page objects | ||
| let minPercentage: number | undefined = undefined; | ||
| const elementTargets: Array<{ selector: string; offsetPx: number }> = []; | ||
|
|
||
| for (const pages of Object.values(this.pageObjects)) { | ||
| for (const page of Object.values(pages)) { | ||
| if (page.trigger_type === 'scrolled_to') { | ||
|
Comment on lines
+424
to
+426
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I see a repeated pattern for each trigger types. Consider refactoring. |
||
| const triggerValue = page.trigger_value as ScrolledToTriggerValue; | ||
|
|
||
| if (triggerValue.mode === 'percent') { | ||
| minPercentage = | ||
| minPercentage === undefined | ||
| ? triggerValue.percentage | ||
| : Math.min(minPercentage, triggerValue.percentage); | ||
| } else if (triggerValue.mode === 'element') { | ||
| const offset = triggerValue.offsetPx || 0; | ||
| // Add if not already present | ||
| if ( | ||
| !elementTargets.some( | ||
| (t) => | ||
| t.selector === triggerValue.selector && t.offsetPx === offset, | ||
| ) | ||
| ) { | ||
| elementTargets.push({ | ||
| selector: triggerValue.selector, | ||
| offsetPx: offset, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Skip setup if no scroll triggers | ||
| if (minPercentage === undefined && elementTargets.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const handleScroll = ( | ||
| minPercent: number | undefined, | ||
| elements: Array<{ selector: string; offsetPx: number }>, | ||
| ) => { | ||
| const scrollPercentage = this.calculateScrollPercentage(); | ||
| const elementAndOffset = new Set<string>(); | ||
|
|
||
| // Check which elements are in range | ||
| for (const { selector, offsetPx } of elements) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: WDYT about using IntersectionObserver for scrolled to element trigger type? |
||
| const element = this.globalScope.document.querySelector(selector); | ||
| if (element) { | ||
| const elementPosition = element.getBoundingClientRect().top; | ||
| const scrollPosition = this.globalScope.scrollY; | ||
|
|
||
| if ( | ||
| scrollPosition + this.globalScope.innerHeight >= | ||
| elementPosition + scrollPosition + offsetPx | ||
| ) { | ||
|
Comment on lines
+470
to
+475
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const key = `${selector}:${offsetPx}`; | ||
| elementAndOffset.add(key); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 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, | ||
| }); | ||
| } | ||
|
Comment on lines
+482
to
+492
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }; | ||
|
|
||
| this.globalScope.addEventListener( | ||
| 'scroll', | ||
| () => handleScroll(minPercentage, elementTargets), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider throttling (e.g. |
||
| { passive: true }, | ||
| ); | ||
|
|
||
| // Check immediately in case user is already scrolled | ||
| handleScroll(minPercentage, elementTargets); | ||
| }; | ||
|
|
||
| private calculateScrollPercentage(): number { | ||
| const windowHeight = this.globalScope.innerHeight; | ||
| const documentHeight = | ||
| this.globalScope.document.documentElement.scrollHeight; | ||
| const scrollTop = this.globalScope.scrollY; | ||
| const scrollableHeight = documentHeight - windowHeight; | ||
|
|
||
| if (scrollableHeight <= 0) { | ||
| return 100; | ||
| } | ||
|
|
||
| return (scrollTop / scrollableHeight) * 100; | ||
| } | ||
|
|
||
| private isPageObjectActive = <T extends MessageType>( | ||
| page: PageObject, | ||
| message: MessagePayloads[T], | ||
|
|
@@ -501,6 +604,23 @@ export class SubscriptionManager { | |
| return this.elementVisibilityState.get(observerKey) ?? false; | ||
| } | ||
|
|
||
| case 'scrolled_to': { | ||
| const triggerValue = page.trigger_value as ScrolledToTriggerValue; | ||
| const scrollPayload = message as ScrolledToPayload; | ||
| const currentScrollPercentage = scrollPayload.scrollPercentage; | ||
| const elementAndOffset = scrollPayload.elementAndOffset; | ||
|
|
||
| if (triggerValue.mode === 'percent') { | ||
| return currentScrollPercentage >= triggerValue.percentage; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
| } else if (triggerValue.mode === 'element') { | ||
| const offset = triggerValue.offsetPx || 0; | ||
| const key = `${triggerValue.selector}:${offset}`; | ||
| return elementAndOffset.has(key); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| default: | ||
| return false; | ||
| } | ||
|
|
||
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.
setupScrolledToPublisheradds a newscrolllistener every timeinitSubscriptions()runs, but never removes the old one. This can leak and double-fire in visual editor mode whereinitSubscriptions()is called repeatedly. Consider removing any previous listener before attaching a new one (or keep a single shared handler).