Set debounceFormSubmitTimer at instance level instead of global #1370
Conversation
NickColley
added
the
🐛 bug
|
Hmm actually on second thoughts because the button component is instantiated at the document level, does this actually fix the issue? @alex-ju 🤔 Right now there's only really one instance at a time right... |
|
Good point, I forgot we're initialising it at the document level. I may need to change that as I don't see other options |
|
@alex-ju for reference I did explore this way of implementing the feature originally: https://github.com/alphagov/govuk-frontend/compare/debounce-button-refactored?expand=1 See the discussion in the original pull request: #1018 (review) |
NickColley
dismissed
their stale review
Alex has changed the implementation based on feedback
| debounceFormSubmitTimer = setTimeout(function () { | ||
| debounceFormSubmitTimer = null | ||
| }, DEBOUNCE_TIMEOUT_IN_SECONDS * 1000) | ||
| this.debounceFormSubmitTimer = setTimeout(function () { |
There was a problem hiding this comment.
Could we write a test to cover the behaviour we want from this change?
There was a problem hiding this comment.
Test added. Do let me know if you see a better/quicker way to test it.
|
Just to update, we will be doing changes to namespacing as part of 3.0.0. We met and discussed the implementation here, and we think we can do a safer implementation alongside the updates we'll have to make for namespacing the data-module syntax. We're going to write up an architecture proposal with the full details. |
|
Thanks for the update, @NickColley! Sounds like a sensible approach. |
|
@NickColley thanks for the work on #1443. I'll update this one. Will need you around for a second review if that's ok with you. |
|
@alex-ju of course :) |
When there are multiple form submit buttons on the page with prevent-double-click enabled they will all reference to the same debounceFormSubmitTimer which means that in a sequence of actions only the first one will called on causing automated end-to-end tests to fail and potentially preventing 'quick' users to be efficient. We're attaching the debounceFormSubmitTimer to each instance so a specific action is stopped from being called multiple times without interfering with other submit button on the page.
|
@NickColley I rebased this on v3 which minimised the number of changes after the prefix was merged. I think this is good for a second review. |
|
Will need a second review too and we can get it merged. |
When there are multiple form submit buttons on the page with
prevent-double-clickenabled they will all reference to the samedebounceFormSubmitTimerwhich means that in a sequence of actions only the first one will be called causing automated end-to-end tests to fail and potentially preventing 'quick' users to be efficient.In this PR we're attaching the
debounceFormSubmitTimerto each instance, so a specific action is stopped from being called multiple times without interfering with other submit buttons on the page.This issue was found while implementing the
prevent-double-clickfeature in govuk_publishing_components so we reverted the change hoping to fix this at the root.P.S. I'm aware WCAG2.1 is a priority for the team now, but I want to add this to the backlog.