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

feat(surveys): some small UI polishing for the survey form editor #22496

Merged
merged 9 commits into from
May 24, 2024
Binary file modified frontend/__snapshots__/scenes-app-surveys--new-survey--light.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodeSurvey.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor Author

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.

import { useEffect } from 'react'
import { NotFound } from 'lib/components/NotFound'
import { SurveyAppearancePreview } from 'scenes/surveys/SurveyAppearancePreview'
Expand Down Expand Up @@ -73,7 +73,7 @@ const Component = ({ attributes }: NotebookNodeProps<NotebookNodeSurveyAttribute
<>
<LemonDivider className="my-0" />
<div className="p-2">
<SurveyReleaseSummary
<SurveyDisplaySummary
id={id}
survey={survey}
targetingFlagFilters={targetingFlagFilters}
Expand Down
13 changes: 9 additions & 4 deletions frontend/src/scenes/surveys/Survey.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
<LemonDivider />
<SurveyEdit />
<LemonDivider />
<SurveyReleaseSummary id={id} survey={survey} targetingFlagFilters={targetingFlagFilters} />
<SurveyDisplaySummary id={id} survey={survey} targetingFlagFilters={targetingFlagFilters} />
<LemonDivider />
<div className="flex items-center gap-2 justify-end">
<LemonButton
Expand All @@ -112,7 +112,7 @@ export function SurveyForm({ id }: { id: string }): JSX.Element {
)
}

export function SurveyReleaseSummary({
export function SurveyDisplaySummary({
id,
survey,
targetingFlagFilters,
Expand All @@ -121,11 +121,16 @@ export function SurveyReleaseSummary({
survey: Survey | NewSurvey
targetingFlagFilters?: FeatureFlagFilters
}): JSX.Element {
const hasConditions = survey.conditions?.url || survey.conditions?.selector
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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 😅

Copy link
Contributor Author

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

const hasFeatureFlags = survey.linked_flag_id || targetingFlagFilters

return (
<div className="flex flex-col mt-2 gap-2">
<div className="font-semibold">Release conditions summary</div>
<div className="font-semibold">Display conditions summary</div>
<span className="text-muted">
By default surveys will be released to everyone unless targeting options are set.
{hasConditions || hasFeatureFlags
? 'Surveys will be displayed to users that match the following conditions:'
: 'Surveys will be displayed to everyone.'}
</span>
{survey.conditions?.url && (
<div className="flex flex-col font-medium gap-1">
Expand Down
12 changes: 7 additions & 5 deletions frontend/src/scenes/surveys/SurveyEdit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,10 @@ export default function SurveyEdit(): JSX.Element {
placeholder="ex: .className or #id"
/>
</LemonField.Pure>
<LemonField.Pure label="Survey wait period">
<LemonField.Pure
label="Survey wait period"
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."
>
Comment on lines +491 to +492
Copy link
Contributor Author

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.

Copy link
Collaborator

@neilkakkar neilkakkar May 24, 2024

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.

<div className="flex flex-row gap-2 items-center">
<LemonCheckbox
checked={!!value?.seenSurveyWaitPeriodInDays}
Expand All @@ -507,11 +510,10 @@ export default function SurveyEdit(): JSX.Element {
}
}}
/>
Do not display this survey to users who have already
seen a survey in the last
Don't show to users who saw a survey within the last
<LemonInput
type="number"
size="small"
size="xsmall"
min={0}
value={value?.seenSurveyWaitPeriodInDays || NaN}
onChange={(val) => {
Expand All @@ -527,7 +529,7 @@ export default function SurveyEdit(): JSX.Element {
})
}
}}
className="w-16"
className="w-12"
/>{' '}
days.
</div>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/surveys/SurveyView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { NodeKind } from '~/queries/schema'
import { InsightType, PropertyFilterType, PropertyOperator, Survey, SurveyQuestionType, SurveyType } from '~/types'

import { SURVEY_EVENT_NAME } from './constants'
import { SurveyReleaseSummary } from './Survey'
import { SurveyDisplaySummary } from './Survey'
import { SurveyAPIEditor } from './SurveyAPIEditor'
import { SurveyFormAppearance } from './SurveyFormAppearance'
import { surveyLogic } from './surveyLogic'
Expand Down Expand Up @@ -227,7 +227,7 @@ export function SurveyView({ id }: { id: string }): JSX.Element {
</>
)}
<LemonDivider />
<SurveyReleaseSummary
<SurveyDisplaySummary
id={id}
survey={survey}
targetingFlagFilters={targetingFlagFilters}
Expand Down
1 change: 1 addition & 0 deletions frontend/src/scenes/surveys/constants.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const defaultSurveyFieldValues = {
scale: 10,
lowerBoundLabel: 'Unlikely',
upperBoundLabel: 'Very likely',
buttonText: 'Submit',
Copy link
Contributor Author

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.

},
],
appearance: {
Expand Down
Loading