feat: add dismiss button to survey report banner - #34160
Conversation
|
Thanks for the pull request, @Alec4r! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
7ca1022 to
7d4129b
Compare
Alec4r
changed the title
|
@robrap @macdiesel @pwnage101 This PR adds the code that was reverted here: #34110 We relocated the script to "base_site.html" and integrated the snippet using the "extrahead" block to mitigate issues with enterprises: See here. |
|
Hello @Alec4r, thanks for this PR. The functionality that adds the dismiss button and shows the thanks message works as expected. @macdiesel @pwnage101 @Alec4r, can you please provide me with some guidance on verifying that the jQuery is not loaded twice? |
|
@MaferMazu great question. To be sure, you can check the comparison between this PR and the code at sha: f52c9c6 (this is right before the PR 34110 was reverted). When doing the comparison you need to navigate to a url of the type: Where you can see the source html of the page and search for jquery |
MaferMazu
left a comment
MaferMazu
left a comment
There was a problem hiding this comment.
Thanks, @felipemontoya, for the explanation.
This is the before and after I get.
This looks good to me. Thanks for the PR, @Alec4r.
What do you think @macdiesel?
felipemontoya
left a comment
felipemontoya
left a comment
There was a problem hiding this comment.
Thanks @Alec4r. I tested this just now and it works as advertised. Thanks for moving the js code to its own file.
I tested the double jquery issue in general pages and the manage_learners page as well and it is gone.
|
I tested again with the changes, and it worked as expected. Thanks @Alec4r 🌟 |
|
@Alec4r 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
* feat: add dismiss button to survey report banner * refactor: move banner script to js file * fix: remove not in conditional
5 participants
Description
This PR introduces a dismiss button to hide the survey report banner and a thank-you message to users upon report submission.
These changes were reverted in this commit: Reverts #34110.
We relocated the script to "base_site.html" and integrated the script snippet the "extrahead" block to mitigate issues with enterprises: See here.
We need to incorporate jQuery in "base_site.html" as the banner should appear across all Django admin pages and if we don't add jquery, our scripts won't work.