Improve documentation and consistency in radios and checkboxes components #1847
Conversation
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1847
| var $content = this.$module.querySelector('#' + $input.getAttribute('aria-controls')) | ||
|
|
||
| if ($content) { |
There was a problem hiding this comment.
Additional suggestion for increasing consistency: radios have a check $content.classList.contains('govuk-radios__conditional') on this line.
There was a problem hiding this comment.
Good spot. I think we were going to do this in #1843 but can include that here now instead.
| * Loop over all items with [data-controls] | ||
| * Check if they have a matching conditional reveal | ||
| * If they do, assign attributes. | ||
| **/ |
There was a problem hiding this comment.
Does this comment definitely need removing? It still feels useful to me - maybe excluding the line
Loop over all items with [data-controls]
Or a different solution might be to rename
var target
to
var conditionalReveal
There was a problem hiding this comment.
I removed it as there's a lot of duplication between this comment and the comments within the callback function.
I've clarified the comments within the function, and I think they cover everything that was in this comment except the fact that it's a loop?
In terms of renaming the variable, the target element that's being associated is probably (hopefully!) a conditional reveal, but I don't think we know that for sure at this point.
There was a problem hiding this comment.
As discussed earlier, we could add a general comment to indicate that the JavaScript here sets up the relationship of the aria-controls and conditional reveals.
the target element that's being associated is probably (hopefully!) a conditional reveal, but I don't think we know that for sure at this point.
True. We could have a comment above that line to convey the intention to extract the conditional reveal attribute (but it might not be necessary to add that if we add a comment elsewhere about the relationship).
There was a problem hiding this comment.
I've added JSDoc comments to each function, aiming to explain what the JS is doing.
@hannalaakso I'd value any thoughts or feedback – do you think this addresses your concerns?
There was a problem hiding this comment.
Yes the comments and clarified function names are super helpful 👍
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1847
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1847
36degrees
force-pushed
the
checkboxes-radios-misc-improvements
branch
from
65cf399 to
5373696
Compare
Building on top of #1842: