fix: make callback on confirmation message instead #1845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: -342 B (-0.01%) Total Size: 3.63 MB
ℹ️ View Unchanged
|
lucasheriques
added
feature/surveys
bump patch
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR fixes a bug where feedback survey confirmation messages were closing prematurely by moving the survey reset functionality to trigger on confirmation message close instead of survey submission.
- Added new test in
src/__tests__/extensions/surveys/survey-popup.test.tsxto verifyonCloseConfirmationMessagecallback behavior - Removed
onPopupSurveySentfromSurveyContextinsrc/extensions/surveys/surveys-utils.tsxin favor of confirmation message close handler - Added comprehensive end-to-end test in
playwright/surveys/feedback-widget.spec.tsfor multiple survey submissions with thank you messages - Improved accessibility in
BottomSection.tsxandQuestionHeader.tsxwith proper ARIA attributes
7 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| previewPageIndex={mockSurvey.questions.length} | ||
| /> | ||
| ) | ||
| const cancelButton2 = screen.getByRole('button', { name: 'Close survey', hidden: true }) |
There was a problem hiding this comment.
logic: The button name 'Close survey' doesn't match the actual button text 'Close' defined in mockSurvey.appearance.thankYouMessageCloseButtonText
Suggested change
| const cancelButton2 = screen.getByRole('button', { name: 'Close survey', hidden: true }) | |
| const cancelButton2 = screen.getByRole('button', { name: mockSurvey.appearance.thankYouMessageCloseButtonText, hidden: true }) |
| // Click the cancel button | ||
| fireEvent.click(cancelButton2) | ||
| // Verify that onCloseConfirmationMessage was called | ||
| expect(mockOnCloseConfirmationMessage).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
style: Should also verify mockRemoveSurveyFromFocus was called to ensure proper cleanup
Comment on lines
819
to
821
| if (nextStep === SurveyQuestionBranchingType.End) { | ||
| sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog) | ||
| onPopupSurveySent() | ||
| } else { |
There was a problem hiding this comment.
logic: Survey sent event is triggered but confirmation state is not set. Add setIsSurveySent(true) here to ensure confirmation message shows.
Suggested change
| if (nextStep === SurveyQuestionBranchingType.End) { | |
| sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog) | |
| onPopupSurveySent() | |
| } else { | |
| if (nextStep === SurveyQuestionBranchingType.End) { | |
| sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog) | |
| window.dispatchEvent(new Event('PHSurveySent')) | |
| } else { |
Comment on lines
+215
to
+248
| // ... existing code ... | ||
| test('auto contrasts text color for feedback tab', async ({ page, context }) => { | ||
| const surveysAPICall = page.route('**/surveys/**', async (route) => { | ||
| await route.fulfill({ | ||
| json: { | ||
| surveys: [ | ||
| { | ||
| id: '123', | ||
| name: 'Test survey', | ||
| type: 'widget', | ||
| start_date: '2021-01-01T00:00:00Z', | ||
| questions: [openTextQuestion], | ||
| appearance: { | ||
| widgetLabel: 'white widget', | ||
| widgetType: 'tab', | ||
| widgetColor: 'white', | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| await start(startOptions, page, context) | ||
| await surveysAPICall | ||
|
|
||
| await expect(page.locator('.PostHogWidget123').locator('.ph-survey-widget-tab')).toBeVisible() | ||
|
|
||
| await expect(page.locator('.PostHogWidget123').locator('.ph-survey-widget-tab')).toHaveCSS('color', black) | ||
| await expect(page.locator('.PostHogWidget123').locator('.ph-survey-widget-tab')).toHaveCSS( | ||
| 'background-color', | ||
| white | ||
| ) | ||
| }) |
There was a problem hiding this comment.
logic: This test is a duplicate of the previous 'auto contrasts text color for feedback tab' test. Should be removed to avoid redundant test coverage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
after I fix I did for this issue, it introduced a bug for feedback surveys where the confirmation message is not shown for feedback survey since it's being closed in the same instant.
so, this PR does two things:
resetfor the feedback widget popup happens when the survey is closedChecklist