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): templates #17904

Merged
merged 26 commits into from
Oct 16, 2023
Merged

feat(surveys): templates #17904

merged 26 commits into from
Oct 16, 2023

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented Oct 11, 2023

Problem

Build some pre defined survey templates

Changes

Screen Shot 2023-10-11 at 2 28 06 AM

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@joethreepwood joethreepwood left a comment

Choose a reason for hiding this comment

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

Following the Slack thread, some suggestions for better/shorter descriptions. Agree with Cory that more than this = docs.

frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
@liyiy
Copy link
Contributor Author

liyiy commented Oct 11, 2023

Screenshots with updated suggestions from @corywatilo

Screen Shot 2023-10-11 at 7 57 05 PM Screen Shot 2023-10-11 at 7 57 18 PM

@liyiy liyiy marked this pull request as ready for review October 11, 2023 23:59
@liyiy
Copy link
Contributor Author

liyiy commented Oct 12, 2023

I'm not releasing this behind a feature flag (unless there's a very good reason to), so I would appreciate a thorough click through review if possible 🙏 😁

@andyvan-ph
Copy link
Contributor

I'd drop the (Superhuman) on the product-market fit survey. AS Cory said, many people won't understand the reference, plus Superhuman didn't actually create the survey.

Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Overall looks great!

Some issues:

  1. Once I create a new survey with template, I can't create a 2nd one using a template.

  2. There's something weird happening sporadically, when if I create a survey, and then go to create a new one, click on any template, I'm redirected to the survey I just created.

  3. The link button says 'Submit' (which goes to Calendly) which isn't very clear. Should call it register

@@ -39,6 +39,7 @@ export const appScenes: Record<Scene, () => any> = {
[Scene.EarlyAccessFeature]: () => import('./early-access-features/EarlyAccessFeature'),
[Scene.Surveys]: () => import('./surveys/Surveys'),
[Scene.Survey]: () => import('./surveys/Survey'),
[Scene.SurveyTemplates]: () => import('./surveys/SurveyTemplates'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the new scene for templates doesn't really make sense anymore, since it's on the surveys page itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually still routes to survey_templates which is its own scene, I was thinking a bit about this on whether to just incorporate it directly onto the surveys components or not but I think a separation of routes is the better option. That way users can always be directed to the templates page instead of it being a "state"

frontend/src/scenes/surveys/SurveyAppearance.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/Surveys.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/constants.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/surveys/surveyLogic.tsx Outdated Show resolved Hide resolved
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 0 modified, 0 deleted
  • webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@corywatilo
Copy link
Contributor

From the latest preview above, the title and description now feel very far apart now that they're shorter. I'd move one of them so they're displayed together.

@liyiy
Copy link
Contributor Author

liyiy commented Oct 16, 2023

From the latest preview above, the title and description now feel very far apart now that they're shorter. I'd move one of them so they're displayed together.

agreed! done

@liyiy liyiy merged commit 93e8ccc into master Oct 16, 2023
73 checks passed
@liyiy liyiy deleted the surveys/templates branch October 16, 2023 20:32
daibhin pushed a commit that referenced this pull request Oct 23, 2023
* routes for survey templates

* basic on click setup logic

* Update UI snapshots for `chromium` (2)

* import type

* Apply suggestions from code review

Co-authored-by: Joe Martin <[email protected]>

* address feedback from cory

* fix logic

* remove unused logic file

* Update frontend/src/scenes/surveys/constants.tsx

Co-authored-by: Joe Martin <[email protected]>

* Update frontend/src/scenes/surveys/constants.tsx

Co-authored-by: Neil Kakkar <[email protected]>

* address comments

* Update UI snapshots for `chromium` (2)

* more fixes

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (2)

* move description and title together

* fix e2e

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joe Martin <[email protected]>
Co-authored-by: Neil Kakkar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants