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

Outsource initialization of Bootstrap popovers (Fix "New comment" button) #534

Merged
merged 9 commits into from
Aug 28, 2023

Conversation

Frodo161
Copy link
Collaborator

This branch fixes issue #524: Open the show_comment page. Hit the "new comment" button, then click "abort". Hitting the "new comment" button again won't have any effect now.

The problem is that two variables in the edit javascript file are declared as constants while hitting the button twice causes them to be redefined.

Changing the variable declaration from "const" to "var" won't cause any problems in this situation and fixes the bug.

…e to have no effect, if pressed multiple times. The reason for this is that the variables popoverTriggerList and popoverList (in the file that is being changed for this commit) are declared as constants. Clicking the button a second time causes the script to run again such that the variables will be defined a second time.
@Frodo161 Frodo161 requested a review from Splines August 23, 2023 13:55
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #534 (1ebeaeb) into mampf-next (883417c) will not change coverage.
Report is 1 commits behind head on mampf-next.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           mampf-next     #534   +/-   ##
===========================================
  Coverage       66.48%   66.48%           
===========================================
  Files             311      311           
  Lines            9417     9417           
===========================================
  Hits             6261     6261           
  Misses           3156     3156           

@Splines Splines changed the title Fix bug which causes the "new comment" button on the show_comment page to have no effect Fix "New comment" button on comments page (init Bootstrap popovers) Aug 23, 2023
@Splines
Copy link
Member

Splines commented Aug 23, 2023

@Frodo161 You're change certainly gets the job done. However, the code you were touching is actually duplicated code. Bootstrap requires to manually initialize popovers. When we dynamically add content to the page, we have to reinitialize this. I outsourced this behavior to a new function in app/assets/javascripts/bootstrap_popovers.js which is called from the respective places, so that we don't have the same code all over the place.

If you are fine with this, please make sure to test that popovers work everywhere, e.g. test popovers at some places, but especially in places where content is loaded dynamically, e.g. when you add a new chapter and a modal opens for that operation that has some popovers (helptexts) in it.

@Splines Splines changed the title Fix "New comment" button on comments page (init Bootstrap popovers) Outsource initialization of Bootstrap popovers (Fix "New comment" button) Aug 23, 2023
@Splines Splines added the bug label Aug 26, 2023
@Splines Splines mentioned this pull request Aug 27, 2023
Copy link
Collaborator Author

@Frodo161 Frodo161 left a comment

Choose a reason for hiding this comment

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

I didn't see anything in the code (except the two missing lines you already added).

In my local MaMpf environment I tested some popovers (chapters, sections, lessons, comments and some more) and everything worked.

@Splines Splines merged commit 7daf91a into mampf-next Aug 28, 2023
7 checks passed
@Splines Splines deleted the fix/hitting-new-comment-has-no-effect branch August 28, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants