[MAJOR] use attribute hidden #691
Conversation
| this.element.setAttribute('data-choice', 'active'); | ||
| } | ||
|
|
||
| reveal() { | ||
| // Reinstate passed element | ||
| this.element.classList.remove(this.classNames.input); | ||
| this.element.classList.remove(this.classNames.hiddenState); | ||
| this.element.hidden = false; |
There was a problem hiding this comment.
Would it not be better to remove the attribute here?
There was a problem hiding this comment.
It's generally bad practice to mix simplified accessors (like .hidden or .tabIndex) with getAttribute/setAttribute. It does the same job, but confuses developers. For example in this case this.element.hidden = false will actually remove the attribute from DOM element, because hidden is a boolean attribute (so, there is no such thing as hidden="false" attribute)
PS: You may push something into this PR, so, GitHub Action will re-run again with Codecov coverage report (it doesn't give access to secrets for forked repositories, as mine)
There was a problem hiding this comment.
Ah didn't realise that will remove it - fair enough 👍
|
Thanks :) |
You are using CSS to hide original element, while it's an exact case for global hidden attribute.
Some very cool organisations also agreed that it's a bad practice.
hiddenattribute is more accessible also, as it impliesaria-hiddenautomatically.This also fixes bug in current version that
.is-hiddenwas defined in optionalbase.scss, not in requiredchoices.scss