feat: allow rendering survey popups on demand #2032
Conversation
lucasheriques
added
bump minor
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Adds the ability to programmatically render survey popups on demand through the PostHog JS client, providing more control over survey display timing.
- Added new public method
renderSurveyPopup(surveyId)to PostHog class for immediate popup rendering without delay/conditions - Made
_handlePopoverSurveypublic and addedignoreDelayparameter in SurveyManager for better control - Comprehensive test suite in
render-on-demand.spec.tscovering edge cases like invalid IDs and multiple renders - Clear separation between styled popups (
renderSurveyPopup) and unstyled surveys (renderSurvey) in the API
4 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
| surveyPopupDelaySeconds: 150, | ||
| }, |
There was a problem hiding this comment.
style: surveyPopupDelaySeconds of 150 (2.5 minutes) seems excessive for a test. Consider using a smaller value like 3-5 seconds
| // Wait for 1 second so survey is dismissed internally | ||
| await page.waitForTimeout(1000) |
There was a problem hiding this comment.
style: Using arbitrary timeout of 1000ms. Consider extracting this to a named constant to make the wait time's purpose more clear
| }) | ||
| }) | ||
|
|
||
| // Survey should be visible immediately (ignoring the 3-second delay) |
There was a problem hiding this comment.
syntax: Comment incorrectly states '3-second delay' when the actual delay is 150 seconds
| // Survey should be visible immediately (ignoring the 3-second delay) | |
| // Survey should be visible immediately (ignoring the 150-second delay) |
| renderSurveyPopup(surveyId: string) { | ||
| if (isNullish(this._surveyManager)) { | ||
| logger.warn('init was not called') | ||
| return | ||
| } | ||
| const survey = this._getSurveyById(surveyId) | ||
| if (!survey) { | ||
| logger.warn('Survey not found') | ||
| return | ||
| } | ||
| this._surveyManager.handlePopoverSurvey(survey, true) | ||
| } |
There was a problem hiding this comment.
logic: should check eligibility with _checkSurveyEligibility before rendering, like other render methods do
|
Size Change: +4.03 kB (+0.11%) Total Size: 3.57 MB
ℹ️ View Unchanged
|
| /** Render a styled survey popup. Notice that this method ignores any delay or conditions set on the survey. */ | ||
| renderSurveyPopup(surveyId: string): void { | ||
| this.surveys.renderSurveyPopup(surveyId) | ||
| } |
There was a problem hiding this comment.
could this be just an extra param to renderSurvey method?
its a bit confusing right now, 2 methods with similar names, and the difference is ignoring delay or conditions
There was a problem hiding this comment.
they can be, but the outcome is very different for both of them, which is why I made them different.
renderSurvey render the survey form fields, unstyled, in a parent container. Nothing else.
now, renderSurveyPopup, or new method, does:
- hooks the survey in the
SurveyManagerfor tracking focus states - render the survey exactly how it's rendered in the Survey Preview. i.e. the floating popover, with all the styles and shadow DOM
- it also ignores both delay and display conditions (answering your question - it ignores all conditions and just displays it)
which is why I decided to make it a separate method. The renderSurvey also needs a selector parameter, which this one doesn't. JS doesn't have overloaded parameters, so we'd also have to change the signature to accept either a selector or styled parameter, and potentially change it to an object.
does this make sense?
There was a problem hiding this comment.
it does, but that required quite a big explanation, i dont think the name renderSurvey vs renderSurveyPopup will be clear for users tbh, UX here is key so people find what they need and dont run into bugs/different behaviours just because they used the wrong method.
There was a problem hiding this comment.
do you have a suggestion? i don't think using the same method is ideal because of the signature change it requires.
maybe we use a name similar to the event we use? the event is survey shown, so we could rename it to showSurvey, wdyt?
There was a problem hiding this comment.
and the difference is ignoring delay or conditions
i thought that was the only difference, so using the same method name does not make sense
renderSurveyPopup would ignore conditions which is not what i would expect and it'd be different than renderSurvey
maybe renderSurveyPopup takes a param to either ignroe or now the conditions, so at least the behaviour is aligned to renderSurvey, default false, so its aligned with renderSurvey
same for renderSurveyPopup
one can:
renderSurvey(..., ignoreConditions: true) // (defualt false)
renderSurveyPopup(..., ignoreConditions: true) // (defualt false)
write better comments on both methods explaining the difference and why we have the new ignoreConditions
thats what i'd think it'd help a bit with a less confusing API/UX. wdyt?
There was a problem hiding this comment.
displaySurvey makes sense to me; there's still a difference between rendering a popup or in a selector.
That can be another optional parameter.
interface DisplaySurveyOptions {
styled: bool // default: true
ignoreDelay: bool // default: false
ignoreConditions: book // default: false
displayType: enum // (popup or inlined) default: popup
selector: string? // default null, requires displayType == inlined
}
something like that?
There was a problem hiding this comment.
cc @ioannisj for opinions since we will need this on mobile most likely in the future
There was a problem hiding this comment.
Yeah, I think the final form of this looks reasonable enough to me. Not sure we want displayType if are passing a selector though?
There was a problem hiding this comment.
I mean what will a displayType: popup with a selector do?
There was a problem hiding this comment.
in this case, ignore the selector
also fine with just the selector, not sure if it will be obvious to users tho, they will have to figure out the difference between having and not having the selector, whereas with a displayType, you can write comments etc
Changes
make it easy to render popup surveys on demand! this way, there's a simpler way for our customers to render surveys whenever they want.
Closes #1981
Checklist