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: recontactSessions new option to remind about survey for some time #2443

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KatSick
Copy link

@KatSick KatSick commented Apr 13, 2024

What does this PR do?

Add a feature to show survey "reminder" based on displays count. E.g. show survey 3 times and then stop.
Currently it is only possible to show once or until feedback.

image

How should this be tested?

  • Add "Recontact session times". Verify formbricks shows that number of times

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
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 🙏

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 13, 2024

@KatSick is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

github-actions bot commented Apr 13, 2024

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

@mattinannt
Copy link
Member

@KatSick Thanks for opening the PR :-) This approach sounds very interesting and I will discuss it with the team :-)
I would do some renaming to make it clearer that the survey will be shown max 5 times per user and also we need to include the check into the getSyncSurveys function, but otherwise it looks good :-)

@KatSick
Copy link
Author

KatSick commented Apr 16, 2024

thanks @mattinannt. added filtering to getSyncSurveys and fix bug with URL forming. we are using formbricks under reverse-proxy under /formbricks endpoint and if we pass /formbricks to the apiHost property, it lose the /formbricks prefix

@jobenjada
Copy link
Member

Hey @KatSick

I have two objections from a UX point of view:

  1. Not in every case session === display. So we should make it dependent on the number of displays, not on the number of sessions

  2. The UI is consistent with our current selection of Recontact options and would create a bunch of edge cases. I suggest the following:

image image

So I suggest something like this:
image

What do you think?

Thanks a lot! :)

@jobenjada jobenjada self-requested a review April 22, 2024 09:14
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 above

@KatSick
Copy link
Author

KatSick commented Apr 22, 2024

looks awesome to me. I will change my implementation.

@jobenjada
Copy link
Member

hey @KatSick - any updated from your end? :)

@KatSick
Copy link
Author

KatSick commented Apr 29, 2024

Working on it. I hope to finish it today/tomorrow

@KatSick KatSick force-pushed the main branch 2 times, most recently from 9c0b416 to 7a060a5 Compare April 30, 2024 09:11
@KatSick
Copy link
Author

KatSick commented Apr 30, 2024

@jobenjada updated

@KatSick
Copy link
Author

KatSick commented May 1, 2024

I noticed, that current logic is broken for "app" type surveys, since they do not have filterSurveys function anymore... investigating

@KatSick
Copy link
Author

KatSick commented May 1, 2024

I tried my best to fix it, however, I does not work correctly still for app surveys, because it does not always re-fetch config in sync.ts file. If I delete the formbricks-js-app localstorage cache (to force update), it works correctly each time.

how this feature should work without local client state as in survey website type?

fix: makeRequest - allow apiHost with baseURL
fix: refactor getSyncSurveys function to filter surveys based on recontactSessions
refactor: use recontactSession as displayOption + fix close button
fix: use responses to detect if user answered to survey
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