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

Make feedback tab survey display responsive #1704

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Make feedback tab survey display responsive #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f96d1d
test: add test for responsive survey tab
bvosk Jan 30, 2025
0917bd0
fix: make survey tab widget responsive
bvosk Jan 30, 2025
02956d1
fix: allow survey to clear feedback button
bvosk Jan 30, 2025
fe326c3
refactor: remove unused reference
bvosk Jan 30, 2025
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
35 changes: 35 additions & 0 deletions playwright/surveys/feedback-widget.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,41 @@ test.describe('surveys - feedback widget', () => {
await pollUntilEventCaptured(page, 'survey sent')
})

test('displays feedback tab in a responsive manner ', async ({ page, context }) => {
const surveysAPICall = page.route('**/surveys/**', async (route) => {
await route.fulfill({
json: {
surveys: [
{
id: '123',
name: 'Test survey',
type: 'widget',
start_date: '2021-01-01T00:00:00Z',
questions: [
{ type: 'open', question: 'Feedback for us?', description: 'tab feedback widget' },
],
appearance: {
widgetLabel: 'Feedback',
widgetType: 'tab',
displayThankYouMessage: true,
thankyouMessageHeader: 'Thanks!',
thankyouMessageBody: 'We appreciate your feedback.',
},
},
],
},
})
})

await start(startOptions, page, context)
await surveysAPICall

await page.locator('.PostHogWidget123').locator('.ph-survey-widget-tab').click()
await page.setViewportSize({ width: 375, height: 667 })

await expect(page.locator('.PostHogWidget123').locator('.survey-form')).toBeInViewport()
})

test('widgetType is custom selector', async ({ page, context }) => {
const surveysAPICall = page.route('**/surveys/**', async (route) => {
await route.fulfill({
Expand Down
20 changes: 7 additions & 13 deletions src/extensions/surveys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '../posthog-surveys-types'

import * as Preact from 'preact'
import { useContext, useEffect, useMemo, useRef, useState } from 'preact/hooks'
import { useContext, useEffect, useMemo, useState } from 'preact/hooks'
import { document as _document, window as _window } from '../utils/globals'
import { createLogger } from '../utils/logger'
import { isNull, isNumber } from '../utils/type-utils'
Expand Down Expand Up @@ -701,7 +701,6 @@ export function FeedbackWidget({
}): JSX.Element {
const [showSurvey, setShowSurvey] = useState(false)
const [styleOverrides, setStyle] = useState({})
const widgetRef = useRef<HTMLDivElement>(null)
Copy link
Author

Choose a reason for hiding this comment

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

I ended up removing this as it's no longer needed. It muddies up the diff quite a bit, but have a look at the individual commits if you'd like to separate this change from the rest.


useEffect(() => {
if (!posthog) {
Expand All @@ -713,17 +712,13 @@ export function FeedbackWidget({
}

if (survey.appearance?.widgetType === 'tab') {
if (widgetRef.current) {
const widgetPos = widgetRef.current.getBoundingClientRect()
const style = {
top: '50%',
left: parseInt(`${widgetPos.right - 360}`),
bottom: 'auto',
borderRadius: 10,
borderBottom: `1.5px solid ${survey.appearance?.borderColor || '#c9c6c6'}`,
}
setStyle(style)
const style = {
top: '50%',
bottom: 'auto',
borderRadius: 10,
borderBottom: `1.5px solid ${survey.appearance?.borderColor || '#c9c6c6'}`,
}
setStyle(style)
}
if (survey.appearance?.widgetType === 'selector') {
const widget = document.querySelector(survey.appearance.widgetSelector || '') ?? undefined
Expand All @@ -741,7 +736,6 @@ export function FeedbackWidget({
{survey.appearance?.widgetType === 'tab' && (
<div
className="ph-survey-widget-tab"
ref={widgetRef}
onClick={() => !readOnly && setShowSurvey(!showSurvey)}
style={{ color: getContrastingTextColor(survey.appearance.widgetColor) }}
>
Expand Down
2 changes: 1 addition & 1 deletion src/extensions/surveys/surveys-utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const logger = createLogger('[Surveys]')
export const style = (appearance: SurveyAppearance | null) => {
const positions = {
left: 'left: 30px;',
right: 'right: 30px;',
right: 'right: 60px;',
Copy link
Author

Choose a reason for hiding this comment

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

Repeating my commit message here for discussion. This wasn't strictly necessary to fix the reported issue, but I believe this fix created this problem of overlap with the feedback button. Perhaps fixing this overlap motivated the style override in the first place. Not sure.

The 30px tucks the survey right in the bottom right corner of the page when it's presented as a popover. But overlaps with the feedback button when presented in that manner.

I'm opting to solve this by adding additional clearance in both configurations in favor of simplicity. It could be handled conditionally, but that would require some more logic.

The "more logic" is more than I originally thought because - the way things are today - a user can configure a popover survey with a appearance.widgetType of tab. So properly handling this requires either adding validation to make that impossible or passing the whole survey - not just SurveyAppearance into this style utility function. Passing on that for now although there's a case for it.

Here's how it looks with the unchanged right: 30px style.

tab_without_margin

center: `
left: 50%;
transform: translateX(-50%);
Expand Down