Remove Multiple Downloads #1428
Conversation
|
@publiclab/is-reviewers |
Codecov Report
@@ Coverage Diff @@
## main #1428 +/- ##
=======================================
Coverage 64.85% 64.85%
=======================================
Files 127 127
Lines 2598 2598
Branches 418 418
=======================================
Hits 1685 1685
Misses 913 913
|
|
@harshkhandeparkar added unbind method and removed the previous method. As the event listener gets triggered each time onComplete is called we need to unbind it from all the previous values. |
examples/lib/defaultHtmlStepUi.js
Outdated
| @@ -308,7 +308,7 @@ function DefaultHtmlStepUi(_sequencer, options) { | |||
| return output.split('/')[1].split(';')[0]; | |||
| } | |||
|
|
|||
| $stepAll('.download-btn').on('click', () => { | |||
| $stepAll('.download-btn').unbind().on('click', () => { | |||
There was a problem hiding this comment.
Wouldn't it be better to use .off('click') instead of unbind? unbind will remove all the event handlers but we only want the click event handler to be removed.
There was a problem hiding this comment.
Also, did you find out why $stepAll is used here? This is a bit confusing. Does it work with just $step? If yes, then it would be better to use $step instead of $stepAll.
Wait, the error could be caused because of $stepAll. We want to add a download listener to the current step but $stepAll will add a listener to every step each time a new step is added. So every time a step is added, an extra listener will be added to the older steps. If I am not wrong then changing $stepAll to $step without unbind or off should fix everything.
There was a problem hiding this comment.
Changing it to $step didn't work.
Isn't $stepAll derived from scopeSelectorAll which should basically mean to select all the instances of the element present inside the scope (i.e in the current step). By that shouldn't $stepAll refers to the all the elements in the current step.
There was a problem hiding this comment.
Oh yeah! I guess I forgot myself. Lol. I was the one who added scopeQuery 😅.
Okay then your code should work fine. Will review again in a min.
There was a problem hiding this comment.
Should I revert back to off(‘click) in this one?
There was a problem hiding this comment.
Yeah, I think you should. Sorry for the late response.
|
Great, thank you!!! |
|
This is super work. We'd love a UI test for this if you're interested, to protect this functionality in the future? #1369 |
|
@jywarren thanks. Would love to do that. |
Fixes #1419
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!