-
Notifications
You must be signed in to change notification settings - Fork 131
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): preact surveys components #964
Conversation
Size Change: +15.9 kB (+2%) Total Size: 860 kB
ℹ️ View Unchanged
|
I added a bunch of e2e tests and also manually went through the flows, but would very much appreciate a second or third look from others! 🙏 |
Great job adding all the e2e tests! 🚀 Suggestion: Split e2e tests into a separate PR, merge it first, so we can confirm they work well with both the old and new code |
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.
Bunch of comments, mostly about code quality rather than the functionality itself. I think its worth the extra effort to make this a little tidier now (otherwise it will likely not happen 🙈 )
Thanks @benjackwhite for the thorough review. I've either added in the fixes or left the less urgent ones as TODOs. Let me know if I've missed anything ! |
* Refactor * Fix * fix? * fix? * option 2 * refactor and fixes --------- Co-authored-by: Neil Kakkar <[email protected]> Co-authored-by: Li Yi <[email protected]>
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 much nicer! Easy to review, easy to see how one would make changes later. This was definitely worth the extra investment from my perspective.
Haven't tested the functionality itself - I would watch this like a hawk after it gets merged and ensure that there are no surprises...
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.
🚀
* Refactor * Fix * fix? * fix? * option 2 * refactor and fixes * wip * only export preview render * read only mode for preview * fix import * add question index preview state * refactor auto text color * remove right css for preview * border radius fix * remove comments * move render into loader-surveys * add test and undo optional param * type fixes --------- Co-authored-by: Ben White <[email protected]> Co-authored-by: Neil Kakkar <[email protected]>
Changes
Converts surveys into components
How to manually test:
Follow the instructions for linking this version of posthog-js to your main posthog branch https://github.com/PostHog/posthog-js?tab=readme-ov-file#developing-together-with-another-project
Test any and all flows including:
Checklist