Report-a-problem JS refactoring #581
Conversation
`window.GOVUK.reportAProblem` has been split into a `ReportAProblem` class (which is responsible for the entire report-a-problem widget) and a `ReportAProblemForm`, which is scoped to just the form. This refactoring makes the code less procedural.
Remove last use of `reportAProblem`
Split ReportAProblemForm and ReportAProblem into two files for clarity.
* Don’t concatenate string, use `\` line break * Move `var` declaration to top * Keep quotes consistent
Improve readability of module
The error and confirmation messages affect HTML beyond the scope of the form module, instead trigger events and allow ReportAProblem module to determine how to handle these situations. * Also use smart quotes for error message
Improve readability and make it clear that there’s an ‘on submit’ listener.
|
/cc @dsingleton |
| url: "/contact/govuk/problem_reports", | ||
| dataType: "json", | ||
| data: this.$form.serialize(), | ||
| success: $.proxy(this.triggerSuccess, this), |
There was a problem hiding this comment.
Any reason to use proxy over Function.prototype.bind? It would be good to move everything to bind where possible for consistency. In this case the bind polyfill is already loaded by the time this code will be run.
|
Added a minor comment about the use of I also think the new structure of ReportAProblem is slightly confusing. It looks like a prototypal object but the only method which is exposed on the created object works like a static method rather than an instance method. The two other methods which, which would be valid instance methods, function ReportAProblem(){}
ReportAProblem.prototype.addToggleLink = function(){
...
this.$container...
}
ReportAProblem.prototype.showConfirmation = function(){
...
this.$container...
}
ReportAProblem.prototype.showError = function(){
...
this.$container...
}This would then also better match the structure and interface you have used for the new |
This is more consistent with the rest of the codebase.
| $toggle.on("click", ".report-a-problem-toggle a", function(evt) { | ||
| $container.toggle(); | ||
| evt.preventDefault(); | ||
| }); |
There was a problem hiding this comment.
You can loose the argument to this method and make it work like an instance method using something like:
ReportAProblem.prototype.addToggleLink = function() {
var $toggle = $('\
<div class="report-a-problem-toggle-wrapper js-footer">\
<p class="report-a-problem-toggle">\
<a href="">Is there anything wrong with this page?</a>\
</p>\
</div>');
this.$container.before($toggle);
$toggle.on("click", ".report-a-problem-toggle a", function(evt) {
this.$container.toggle();
evt.preventDefault();
}.bind(this));
}
This work is done in preparation to implementing an alternative design.
This was worked on by @binaryberry, @fofr and myself.