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): replace surveys preview #20321

Merged
merged 47 commits into from
Apr 29, 2024
Merged

feat(surveys): replace surveys preview #20321

merged 47 commits into from
Apr 29, 2024

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented Feb 13, 2024

Problem

Changes

remake of #20280

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

How did you test this code?

@liyiy liyiy requested a review from neilkakkar February 14, 2024 16:23
@liyiy liyiy requested a review from jurajmajerik February 15, 2024 18:33
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

28 snapshot changes in total. 0 added, 28 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

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

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

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.

@jurajmajerik
Copy link
Contributor

Changes:

  1. Fixed guardAvailableFeature used for triggering subscription modal (was moved to another logic in the meantime)
  2. Removed e2e test for the open-ended answer choice. This field is currently not getting auto-selected on text input, which is not a great UX, but we can live with it for a bit longer. Fixing this is beyond the scope of this sprint, but I've created an issue for it.
  3. Adjusted rendering (see fix(surveys): use correct render method posthog-js#1157)

@jurajmajerik jurajmajerik requested review from neilkakkar and removed request for neilkakkar April 26, 2024 10:19
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 4 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, 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.

@neilkakkar
Copy link
Collaborator

One blocking thing I mentioned in the other PR: The preview hides the 'save as draft' button. Otherwise, will give it a final test once the other PR is merged, should be ready to go right after 👍

@jurajmajerik
Copy link
Contributor

Ready for the final check!

image

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Apr 29, 2024

Nice!

Long names bork preview ->
image
.whitespace-nowrap seems to be the sus class

html links seem to show up red in the preview for some reason -> - ah most probs because posthog styles override the preview styles

image

We should center the preview ->
image

CSAT emojis look nicer on prod -> (this is prod)
image
and local ->
image

Seems pretty good other than this 👍

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@jurajmajerik
Copy link
Contributor

I've fixed the alignment issues. Can we leave the color of the CSAT emoji for a follow-up? It seems it has to do with how we're attaching stylesheets, but debugging in the current workflow is 😩

I've confirmed the colors in the edit form and in the live survey are working correctly.

}
return '-inf'
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this based on master correctly 🤔 , iirc this if-else change should already be in master

Copy link
Contributor

Choose a reason for hiding this comment

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

},
{
label: 'Rating',
value: SurveyQuestionType.Rating,
tooltip: () => (
<SurveyRatingAppearance
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh interesting, we're removing these, but would be nice to bring back in a future refactor. I'm assuming this was removed because the preview is now coming from the js package

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue created: #21941

@@ -168,60 +167,6 @@ describe('Surveys', () => {
cy.get('.Toastify__toast-body').contains('Survey deleted').should('be.visible')
})

it('creates a new multiple choice survey with an open-ended choice', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't remember the context for deleting this, but if its something to be fixed in the future, can we instead do it.skip() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created an issue and saved the deleted test in a Gist attached: #21877

But it.skip() is handy, will keep in mind for the next time 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah perf

@jurajmajerik jurajmajerik merged commit 43c5c9f into master Apr 29, 2024
103 checks passed
@jurajmajerik jurajmajerik deleted the update-preview branch April 29, 2024 14:29
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.

4 participants