Skip to content
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.

Sign up for GitHub

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

feat: Support custom scroll selector #961

Merged
merged 2 commits into from
Jan 15, 2024
Merged

feat: Support custom scroll selector #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e9366b
Support custom scroll selector
robbie-c Jan 15, 2024
7b4ab5e
Add a comment for scroll_root_selector
robbie-c Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions playground/nextjs/pages/_app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ if (typeof window !== 'undefined') {
},
debug: true,
__preview_send_client_session_params: true,
__preview_measure_pageview_stats: true,
scroll_root_selector: ['#scroll_element', 'html'],
Copy link
Member Author

@robbie-c robbie-c Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Quick note that you can't use the css comma here, i.e. "#scroll_element,html" as it would always return the html root element, even if #scroll_element is present on the page :)

})
;(window as any).posthog = posthog
}
Expand Down
19 changes: 10 additions & 9 deletions playground/nextjs/pages/_document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { Html, Head, Main, NextScript } from 'next/document'
import React from 'react'

export default function Document() {
return (
<Html lang="en">
<Head />
<body>
<Main />
<NextScript />
</body>
</Html>
)
return (
<Html lang="en">
<Head />
<body>
<Main />
<NextScript />
</body>
</Html>
)
}
1 change: 1 addition & 0 deletions playground/nextjs/pages/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export default function Home() {
<Link href="/iframe">Iframe</Link>
<Link href="/media">Media</Link>
<Link href="/long">Long</Link>
<Link href="/longmain">Long Main</Link>
</div>

<p>Feature flag response: {JSON.stringify(result)}</p>
Expand Down
70 changes: 70 additions & 0 deletions playground/nextjs/pages/longmain.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import Head from 'next/head'
import Link from 'next/link'
import React, { useEffect } from 'react'

export default function Home() {
useEffect(() => {
const html = document.querySelector('html')
const body = document.querySelector('body')
const nextRoot = document.querySelector<HTMLDivElement>('div#__next')
if (!html || !body || !nextRoot) return
html.style.height = '100%'
html.style.overflow = 'hidden'
body.style.height = '100%'
nextRoot.style.height = '100%'
return () => {
html.style.height = ''
html.style.overflow = ''
body.style.height = ''
nextRoot.style.height = ''
}
}, [])

return (
<>
<Head>
<title>PostHog</title>
<meta name="viewport" content="width=device-width, initial-scale=1" />
</Head>
<main
id="scroll_element"
style={{
height: '100%',
overflow: 'scroll',
margin: 0,
padding: 0,
}}
>
<div
style={{
height: '4000px',
overflow: 'hidden',
}}
>
<h1>A long page</h1>
<p>
The window itself does not scroll, the <code>main</code> element does. The content is exactly
4000px tall.
</p>
<div className="buttons">
<Link href="/">Home</Link>
</div>

{Array.from({ length: 100 }, (_, i) => (
<p key={i}>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut
labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco
laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in
voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat
cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
</p>
))}

<div className="buttons">
<Link href="/">Home</Link>
</div>
</div>
</main>
</>
)
}
4 changes: 4 additions & 0 deletions playground/nextjs/styles/globals.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ main {
font-family: helvetica, arial, sans-serif;
}

html, body {
margin: 0
}

.buttons {
display: flex;
gap: 0.5rem;
Expand Down
12 changes: 8 additions & 4 deletions src/__tests__/page-view.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { PageViewManager } from '../page-view'
import { PostHog } from '../posthog-core'

const mockWindowGetter = jest.fn()
jest.mock('../utils/globals', () => ({
Expand All @@ -10,6 +11,9 @@ jest.mock('../utils/globals', () => ({

describe('PageView ID manager', () => {
describe('doPageView', () => {
const instance: PostHog = {
config: {},
} as any
beforeEach(() => {
mockWindowGetter.mockReturnValue({
location: {
Expand Down Expand Up @@ -41,7 +45,7 @@ describe('PageView ID manager', () => {
},
})

const pageViewIdManager = new PageViewManager()
const pageViewIdManager = new PageViewManager(instance)
pageViewIdManager.doPageView()

// force the manager to update the scroll data by calling an internal method
Expand Down Expand Up @@ -72,7 +76,7 @@ describe('PageView ID manager', () => {
},
})

const pageViewIdManager = new PageViewManager()
const pageViewIdManager = new PageViewManager(instance)
pageViewIdManager.doPageView()

// force the manager to update the scroll data by calling an internal method
Expand All @@ -90,7 +94,7 @@ describe('PageView ID manager', () => {
})

it('can handle scroll updates before doPageView is called', () => {
const pageViewIdManager = new PageViewManager()
const pageViewIdManager = new PageViewManager(instance)

pageViewIdManager._updateScrollData()
const firstPageView = pageViewIdManager.doPageView()
Expand All @@ -101,7 +105,7 @@ describe('PageView ID manager', () => {
})

it('should include the pathname', () => {
const pageViewIdManager = new PageViewManager()
const pageViewIdManager = new PageViewManager(instance)

const firstPageView = pageViewIdManager.doPageView()
expect(firstPageView.$prev_pageview_pathname).toBeUndefined()
Expand Down
49 changes: 41 additions & 8 deletions src/page-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { window } from './utils/globals'
import { PostHog } from './posthog-core'
import { _isArray } from './utils/type-utils'

interface PageViewData {
pathname: string
Expand Down Expand Up @@ -32,6 +34,11 @@ interface PageViewEventProperties extends ScrollProperties {
export class PageViewManager {
_pageViewData: PageViewData | undefined
_hasSeenPageView = false
_instance: PostHog

constructor(instance: PostHog) {
this._instance = instance
}

_createPageViewData(): PageViewData {
return {
Expand Down Expand Up @@ -134,8 +141,11 @@ export class PageViewManager {
}

startMeasuringScrollPosition() {
window?.addEventListener('scroll', this._updateScrollData)
window?.addEventListener('scrollend', this._updateScrollData)
// setting the third argument to `true` means that we will receive scroll events for other scrollable elements
// on the page, not just the window
// see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#usecapture
window?.addEventListener('scroll', this._updateScrollData, true)
window?.addEventListener('scrollend', this._updateScrollData, true)
window?.addEventListener('resize', this._updateScrollData)
Comment on lines +144 to 149
Copy link
Member

Choose a reason for hiding this comment

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

trying to think defensively (so feel free to shoot this down) is it worth debouncing here?

the processing looks cheap but we'll get a lot of updates processed that we don't need 🤷

}

Expand All @@ -145,22 +155,45 @@ export class PageViewManager {
window?.removeEventListener('resize', this._updateScrollData)
}

_scrollElement(): Element | null | undefined {
if (this._instance.config.scroll_root_selector) {
const selectors = _isArray(this._instance.config.scroll_root_selector)
? this._instance.config.scroll_root_selector
: [this._instance.config.scroll_root_selector]
for (const selector of selectors) {
const element = window?.document.querySelector(selector)
if (element) {
return element
}
}
return undefined
} else {
return window?.document.documentElement
}
}

_scrollHeight(): number {
return window
? Math.max(0, window.document.documentElement.scrollHeight - window.document.documentElement.clientHeight)
: 0
const element = this._scrollElement()
Copy link
Member

Choose a reason for hiding this comment

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

similar nit on these helpers, we could lookup the element once and pass it into each of these helpers...

(may be so fast it doesn't matter :))

return element ? Math.max(0, element.scrollHeight - element.clientHeight) : 0
}

_scrollY(): number {
return window ? window.scrollY || window.pageYOffset || window.document.documentElement.scrollTop || 0 : 0
if (this._instance.config.scroll_root_selector) {
const element = this._scrollElement()
return (element && element.scrollTop) || 0
} else {
return window ? window.scrollY || window.pageYOffset || window.document.documentElement.scrollTop || 0 : 0
}
}

_contentHeight(): number {
return window?.document.documentElement.scrollHeight || 0
const element = this._scrollElement()
return element?.scrollHeight || 0
}

_contentY(): number {
const clientHeight = window?.document.documentElement.clientHeight || 0
const element = this._scrollElement()
const clientHeight = element?.clientHeight || 0
return this._scrollY() + clientHeight
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/posthog-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export class PostHog {

this.featureFlags = new PostHogFeatureFlags(this)
this.toolbar = new Toolbar(this)
this.pageViewManager = new PageViewManager()
this.pageViewManager = new PageViewManager(this)
this.surveys = new PostHogSurveys(this)
this.rateLimiter = new RateLimiter()

Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ export interface PostHogConfig {
segment?: any
__preview_measure_pageview_stats?: boolean
__preview_send_client_session_params?: boolean
// Let the pageview scroll stats use a custom css selector for the root element, e.g. `main`
scroll_root_selector?: string | string[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could add a doc comment here so it is clear what it does (given the fact that documentation might come much later and this is an edge case option)

}

export interface OptInOutCapturingOptions {
Expand Down
Loading