-
-
Notifications
You must be signed in to change notification settings - Fork 387
Fix/feedback toggle behavior #2043
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
base: main
Are you sure you want to change the base?
Fix/feedback toggle behavior #2043
Conversation
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2043 +/- ##
===========================================
- Coverage 100.00% 99.53% -0.47%
===========================================
Files 30 30
Lines 633 644 +11
Branches 196 199 +3
===========================================
+ Hits 633 641 +8
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @Siddhartha20242 , thank you for the effort. Before raising a PR, the issue needs to pass triage. We require contributors to wait for assignment before raising a PR. |
Utkarsh-123github
left a comment
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.
Unwanted changes in package-lock.json and yarn.lock.
Also please make sure that your PR passes all the checks.
Thank you!
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
3 similar comments
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Hi @Siddhartha20242! Thanks a lot for your contribution! I noticed that the following required information is missing or incomplete: kind of change description Please update the PR description to include this information. You can find placeholders in the PR template for these items. Thanks a lot! |
|
Hi @Utkarsh-123github , I have fixed the PR description and reverted the accidental lockfile changes. So sorry for that. As I am a new contributor. Also, sorry for raising pr without getting assigned. As of now, only the relevant code for the toggle fix is now included. I see that Codecov is at 75% because the existing tests dont cover the double click toggle logic. Would you like me to add a unit test for this, or can we proceed with the manual verification provided in the video? Thank you so much for the guidance. |
|
Yes please fix the tests part as well. |
|
Hi @Utkarsh-123github , I've updated the PR with the requested changes, but I'm hitting a race condition in the DocsHelp toggle test on the CI environment. The test fails because the second click happens before React finishes the first state update. I can fix this locally by adding a small wait or check, but the linter blocks cy.wait() and other assertions seem flaky in the CI pipeline. Do you have a preferred way to handle these toggle race conditions in this repo? I want to make sure I implement it according to your standards. I am worried what to do. I am seeing stack overflow, claude all those from past 4 hours. |
What kind of change does this PR introduce?
Issue Number:
Closes #2042
Screenshots/videos:
https://github.com/user-attachments/assets/9bb29c49-beb4-4ad0-94f1-9735fdd55760
If relevant, did you update the documentation?
N/A
Summary:
Currently, the feedback buttons (Like/Dislike) in the footer only support a "click-to-open" state. If a user clicks a button accidentally, they cannot click the same button again to toggle the form closed. This PR introduces a toggle mechanism using a selectedvote state, allowing the user to dismiss the feedback form by clicking the active button again.
Does this PR introduce a breaking change?
No.
Checklist