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

fix: dont require double click to show feedback surveys #1767

Merged

Conversation

lucasheriques
Copy link
Contributor

Changes

adds a new option on SurveyContext to do something after a survey is submitted.

also, right now, for widget-type surveys (tab style), we have to click on the button twice to show the survey again after dismissing it. this PR fixes that

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

@lucasheriques lucasheriques added bump patch Bump patch version when this PR gets merged feature/surveys labels Feb 27, 2025
@lucasheriques lucasheriques self-assigned this Feb 27, 2025
@lucasheriques lucasheriques requested a review from a team as a code owner February 27, 2025 04:27
Copy link

vercel bot commented Feb 27, 2025

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Feb 27, 2025 5:31pm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds a new onSurveyDismissedOrSent callback function to the SurveyContext to fix an issue with widget-type surveys requiring double clicks to reappear after dismissal.

  • Added onSurveyDismissedOrSent callback to SurveyContext interface in src/extensions/surveys/surveys-utils.tsx with a default no-op implementation
  • Implemented the callback in SurveyPopup component in src/extensions/surveys.tsx to properly handle survey state
  • Added callback invocation when surveys are dismissed or completed to reset widget state
  • Fixed the issue where widget-type surveys required two clicks to show again after being dismissed

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

github-actions bot commented Feb 27, 2025

Size Change: +1.96 kB (+0.06%)

Total Size: 3.32 MB

Filename Size Change
dist/all-external-dependencies.js 218 kB +224 B (+0.1%)
dist/array.full.es5.js 271 kB +268 B (+0.1%)
dist/array.full.js 374 kB +224 B (+0.06%)
dist/array.full.no-external.js 372 kB +224 B (+0.06%)
dist/array.js 183 kB +25 B (+0.01%)
dist/array.no-external.js 182 kB +25 B (+0.01%)
dist/main.js 184 kB +25 B (+0.01%)
dist/module.full.js 374 kB +224 B (+0.06%)
dist/module.full.no-external.js 372 kB +224 B (+0.06%)
dist/module.js 183 kB +25 B (+0.01%)
dist/module.no-external.js 182 kB +25 B (+0.01%)
dist/surveys-preview.js 70.3 kB +224 B (+0.32%)
dist/surveys.js 75 kB +224 B (+0.3%)
ℹ️ View Unchanged
Filename Size
dist/customizations.full.js 14 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.51 kB
dist/external-scripts-loader.js 2.64 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

Copy link

@ioannisj ioannisj left a comment

Choose a reason for hiding this comment

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

LG. Just a suggestion/observation - We already have handleCloseSurveyPopup can we use that for dismissing and just add a new onSurveySent?

@marandaneto
Copy link
Member

LG. Just a suggestion/observation - We already have handleCloseSurveyPopup can we use that for dismissing and just add a new onSurveySent?

yeah I wonder if handleCloseSurveyPopup is called for dismissed/sent, just one of them? which one? are we duplicating something here?

@lucasheriques
Copy link
Contributor Author

@marandaneto @ioannisj

  • handleCloseSurveyPopup is called for dismissing the survey only (survey dismissed)

  • onNextButtonClick is actually the one that is called for survey sent, but it's defined down in the component tree.

I agree this is probably confusing, and there's a refactor somewhere we can do to make the logic be more sound

but it's not on the scope of this PR specifically - where I'm mainly looking for adding a callback that will be triggered on both cases (dismissing or sending the survey)

@marandaneto
Copy link
Member

@marandaneto @ioannisj

  • handleCloseSurveyPopup is called for dismissing the survey only (survey dismissed)
  • onNextButtonClick is actually the one that is called for survey sent, but it's defined down in the component tree.

I agree this is probably confusing, and there's a refactor somewhere we can do to make the logic be more sound

but it's not on the scope of this PR specifically - where I'm mainly looking for adding a callback that will be triggered on both cases (dismissing or sending the survey)

I think the confusion here is that there are one action and 2 callback options
handleCloseSurveyPopup -> dismissing
onSurveyDismissedOrSent -> dismissing and sending

I'd rather do
handleCloseSurveyPopup which exists
handleSentSurveyPopup sending
whenever needed, call both functions (if you need both actions), how does that sound?

Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left a last question here
i might be misunderstanding the behaviour of both callbacks, so feel free to explain and merge it if that's the case

@lucasheriques
Copy link
Contributor Author

@marandaneto @ioannisj i made the callbacks be more clear now

please take another look before I merge 🙏

const {
isPreviewMode,
previewPageIndex,
onPopupSurveyDismissed: handleCloseSurveyPopup,
Copy link
Member

Choose a reason for hiding this comment

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

isnt handleCloseSurveyPopup renamed?

Copy link
Contributor Author

@lucasheriques lucasheriques Feb 27, 2025

Choose a reason for hiding this comment

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

thats what I get from trusting the automatic renaming; i'll fix all of those and merge

{isPopup && (
<Cancel
onClick={() => {
handleCloseSurveyPopup()
Copy link
Member

Choose a reason for hiding this comment

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

onPopupSurveyDismissed: handleCloseSurveyPopup,
isPopup,
onPreviewSubmit,
onPopupSurveySent: onSurveyDismissedOrSent,
Copy link
Member

Choose a reason for hiding this comment

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

isnt onSurveyDismissedOrSent renamed?

@@ -1071,6 +1073,81 @@ describe('useHideSurveyOnURLChange', () => {
})
})

describe('onSurveyDismissedOrSent callback', () => {
Copy link
Member

Choose a reason for hiding this comment

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

tests are using the old callback names

@lucasheriques lucasheriques merged commit 4d23a69 into main Feb 27, 2025
13 of 25 checks passed
@lucasheriques lucasheriques deleted the fix/dont-require-double-click-to-show-feedback-survey branch February 27, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged feature/surveys
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants