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: Enable Prefilling of several values #2482

Merged
merged 19 commits into from
Apr 29, 2024

Conversation

gupta-piyush19
Copy link
Contributor

@gupta-piyush19 gupta-piyush19 commented Apr 18, 2024

What does this PR do?

Enables Prefilling of several values

Fixes
https://github.com/formbricks/internal/issues/73
#2433

multi-question-prefill.mp4

How should this be tested?

  • Create a survey with more than 1 question, try to prefil multiple question by providing id=value in the query params.

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Apr 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
formbricks-cloud ⬜️ Ignored (Inspect) Visit Preview Apr 29, 2024 8:50am
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Apr 29, 2024 8:50am

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Thank you for following the naming conventions for pull request titles! 🙏

@jobenjada jobenjada self-requested a review April 19, 2024 07:38
@jobenjada jobenjada marked this pull request as ready for review April 19, 2024 08:53
@jobenjada jobenjada marked this pull request as draft April 19, 2024 08:54
Copy link
Member

@jobenjada jobenjada left a comment

Choose a reason for hiding this comment

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

Hey! Works well so far :) A few questions:

  1. Please update the Prefilling docs in the link survye section with the changes. Add a section where you explain the format needed for each question type as well as potential edge cases, what happens when the validation doesn't work and add an example for all question types which can be prefilled.
  2. Currently, it's not possible to get the initial UX of clicking on an answer in an email which then gets prefilled and the question is skipped. My idea was to do use logic jumps to still enable this UX, even if not by default (see image). Is this possible without being too hacky? So essentially: Don't skip prefilled values unless a logic jump says so?
  3. Not sure if it was part of the ticket but is it possible to introduce another link parameter skipPrefill which when set to true skips prefilled questions? How hacky would it be?

Thanks!

@gupta-piyush19 gupta-piyush19 marked this pull request as ready for review April 23, 2024 08:55
@gupta-piyush19
Copy link
Contributor Author

Thanks, @jobenjada, for the review.
As per your suggestions,

  1. I've updated the docs and added examples for all the questions that can be prefilled.
  2. We're keeping the logic very simple in this PR, just to show the prefilled value to the user. This could create messed-up logic if done with startAt functionality.
  3. To support skipPrefill, we would still need two different urls, one with skipPrefill query params and the other without them. If we've got to create two different links anyway, it'd be better to just remove the prefiling parameters in the second one instead of adding one extra param.

Hey! Works well so far :) A few questions:

  1. Please update the Prefilling docs in the link survye section with the changes. Add a section where you explain the format needed for each question type as well as potential edge cases, what happens when the validation doesn't work and add an example for all question types which can be prefilled.
  2. Currently, it's not possible to get the initial UX of clicking on an answer in an email which then gets prefilled and the question is skipped. My idea was to do use logic jumps to still enable this UX, even if not by default (see image). Is this possible without being too hacky? So essentially: Don't skip prefilled values unless a logic jump says so?
  3. Not sure if it was part of the ticket but is it possible to introduce another link parameter skipPrefill which when set to true skips prefilled questions? How hacky would it be?

Thanks!

@vercel vercel bot temporarily deployed to Preview – formbricks-com April 23, 2024 14:16 Inactive
@vercel vercel bot temporarily deployed to Preview – formbricks-cloud April 23, 2024 14:16 Inactive
@vercel vercel bot temporarily deployed to Preview – formbricks-cloud April 23, 2024 14:41 Inactive
@vercel vercel bot temporarily deployed to Preview – formbricks-com April 23, 2024 14:41 Inactive
@jobenjada
Copy link
Member

jobenjada commented Apr 23, 2024

Hey Piyush!

Works great, good job until here!

As you mentioned, once you bring startAt to the party, things get messy. Let's have a chat re this tomorrow and then merge this 💪

In the meanwhile: if related to this, pls look into the failed E2E test

Thanks!

…73-feature-enable-prefilling-of-several-values
@jobenjada
Copy link
Member

Hey! A few bugs:

The first question gets a "back" button on prefill:

image image

And the second question doesn't?
image

That should be it :)

Copy link
Member

@jobenjada jobenjada left a comment

Choose a reason for hiding this comment

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

pls see comment

@gupta-piyush19
Copy link
Contributor Author

@jobenjada Thanks for testing the feature. I've pushed the fix for the issues.

prefiling.mp4

Copy link
Contributor

@pandeymangg pandeymangg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @gupta-piyush19 😊 looks great!
I just found this bug while testing, could you please take a look at it 🙏

Screen.Recording.2024-04-29.at.12.04.06.PM.mov

Copy link
Contributor

@pandeymangg pandeymangg left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@jobenjada jobenjada added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit e95a8c7 Apr 29, 2024
12 checks passed
@jobenjada jobenjada deleted the 73-feature-enable-prefilling-of-several-values branch April 29, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants