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.

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

fix: survey URL targeting should re-evaluate after the URL changes #1695

Merged

Conversation

lucasheriques
Copy link
Contributor

@lucasheriques lucasheriques commented Jan 27, 2025

Changes

Adds a new effect to SurveyPopup to continuously check if the URL has changed to display a survey or not.

Right now, if you add a URL filter, go there, and then change the page, the survey is still persisted on the UI.

But, to match the filter properly, it should instead be hidden, as that survey is only supposed to show on a specific URL.

@dmarticus tagged you as reviewer because maybe there's some historical reason as why it was made to work like that instead of hiding it once the URL changes

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Jan 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Jan 28, 2025 6:55pm

Copy link

github-actions bot commented Jan 27, 2025

Size Change: +19.8 kB (+0.61%)

Total Size: 3.29 MB

Filename Size Change
dist/all-external-dependencies.js 215 kB +7.57 kB (+3.65%)
dist/array.full.es5.js 267 kB +700 B (+0.26%)
dist/array.full.js 370 kB +664 B (+0.18%)
dist/array.full.no-external.js 368 kB +664 B (+0.18%)
dist/array.js 182 kB +27 B (+0.01%)
dist/array.no-external.js 180 kB +27 B (+0.01%)
dist/main.js 182 kB +27 B (+0.01%)
dist/module.full.js 370 kB +664 B (+0.18%)
dist/module.full.no-external.js 368 kB +664 B (+0.18%)
dist/module.js 182 kB +27 B (+0.01%)
dist/module.no-external.js 180 kB +27 B (+0.01%)
dist/surveys-preview.js 68.7 kB +1.19 kB (+1.77%)
dist/surveys.js 71.7 kB +7.57 kB (+11.81%) ⚠️
ℹ️ View Unchanged
Filename Size
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

@lucasheriques lucasheriques requested review from dmarticus and a team January 27, 2025 21:00
@lucasheriques lucasheriques marked this pull request as ready for review January 27, 2025 21:03
@lucasheriques lucasheriques marked this pull request as draft January 27, 2025 23:28
Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

looks good, just this comment otherwise LGTM

@lucasheriques lucasheriques added the bump patch Bump patch version when this PR gets merged label Jan 28, 2025
@lucasheriques lucasheriques merged commit dba496c into main Jan 28, 2025
31 checks passed
@lucasheriques lucasheriques deleted the fix/survey-url-targetting-reevaluation-on-url-changes branch January 28, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants