-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(surveys): some small UI polishing for the survey form editor #22496
Conversation
@@ -11,7 +11,7 @@ import { buildFlagContent } from './NotebookNodeFlag' | |||
import { surveyLogic } from 'scenes/surveys/surveyLogic' | |||
import { StatusTag } from 'scenes/surveys/Surveys' | |||
import { SurveyResult } from 'scenes/surveys/SurveyView' | |||
import { SurveyReleaseSummary } from 'scenes/surveys/Survey' | |||
import { SurveyDisplaySummary } from 'scenes/surveys/Survey' |
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.
renamed this since it felt more accurate to reflect the new name of the header.
info="Note that this condition will only apply to one browser for a given non-anonymous user. If the user switches browsers or uses an incognito session, they could see this survey again." | ||
> |
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.
I kinda just fired from the hip on this copy, open to suggestions if folks have any.
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.
this is a bug [well, the same user identity switching browsers bit] 😅 , @Phanatic we found yet another candidate for the internal_targeting_flag
so this works cross-browser.
With a different anonymous user, absolutely makes sense.
@@ -67,6 +67,7 @@ export const defaultSurveyFieldValues = { | |||
scale: 10, | |||
lowerBoundLabel: 'Unlikely', | |||
upperBoundLabel: 'Very likely', | |||
buttonText: 'Submit', |
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.
this was the fix for issue (3) from the description; we just missed a default.
Size Change: +58 B (+0.01%) Total Size: 1.03 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated9 snapshot changes in total. 0 added, 9 modified, 0 deleted:
Triggered by this commit. |
@@ -121,11 +121,16 @@ export function SurveyReleaseSummary({ | |||
survey: Survey | NewSurvey | |||
targetingFlagFilters?: FeatureFlagFilters | |||
}): JSX.Element { | |||
const hasConditions = survey.conditions?.url || survey.conditions?.selector |
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.
For a bonus, would be cool if we can add a line for the wait period here as well, since technically if wait period is set it may not show to everyone.
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.
ooh, great context! Will add.
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.
It also requires adding a bit of text, right now I think when wait period is set the summary shows up empty 😅
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.
oh man, I'm so sorry I missed that. Out of character for me; I should've double-checked. Here's a fix: #22532
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.
Nicely done!
…tHog/posthog into feat/survey-editor-form-improvements
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
This PR partially closes #22283, this covers points 1, 2, 3, and 5 (point 4 is a more involved bit of code that requires interop with posthog-js, so that will be handled in a separate PR).
Please find the specific changes below:
Changes
Before (no conditions applied)
After (no conditions applied)
Before (conditions applied)
After (conditions applied)
I think my changes improve the overall copy and effectiveness of this summary section.
Before:
After:
Before:
After:
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yeah
How did you test this code?
I didn't really change any contracts or functionality, mostly just copy and UI changes. See the images for what I did specifically. Low-risk change IMO.