Add toBeRequired() method #109
Conversation
There was a problem hiding this comment.
Looking good. However I do have some questions about the strictness of validating the tag name. See my comments.
Regarding the type definitions, this certainly needs work on that as well. It's simple though, probably just needing to add these couple of lines to extend-expect.d.ts in the root of the repository.
toBeDisabled(): R
toBeEnabled(): R
+ toBeOptional(): R
+ toBeRequired(): R
toContainElement(element: HTMLElement | SVGElement | null): R
src/to-be-required.js
Outdated
|
|
||
| function isElementRequiredByARIA(element) { | ||
| return ( | ||
| FORM_TAGS.includes(getTag(element)) && |
There was a problem hiding this comment.
On the other hand I suspect that aria-selected is not as restricted, and may be usable even in div elements with certain roles (ref). Though again, I'm not suggesting we go as far as ensuring that if present the element has one of these roles (unless you think is easily doable and makes sense). But at least we should probably not enforce that the element tag is one of these.
There was a problem hiding this comment.
We could enforce that the attribute is present on either:
- a block having
role="combobox",role="gridcell",role="radiogroup",role="spinbutton"orrole="tree" - a block having or a form element corresponding to the
role="listbox"(so any<select>) - an block having or a form element corresponding to the
role="textbox"(so any<input>and<textarea>)
|
Also, I wonder if I will not remove |
This update follows the discussion in the [PR testing-library#109](testing-library#109). `.toBeRequired()` now checks: * the type of input to not return true on unsupported input types * the roles of a DOM node to not return true on unsupported roles * a reduced number of supported inputs following the HTML specification
Document these two methods according to the new test scenarii
|
@gnapse New commits updating the PR with the requested changes. Also, I checked the code coverage & added the type definition. |
There was a problem hiding this comment.
This looks good now. I too tend to think as you do, that we can leave toBeOptional out. One can always use .not.toBeRequired. The reason we provided .toBeEnabled is because the word "disabled" is a negation, so .not.toBeDisabled tends to sound like a double negation. But this is not the case with the word "required".
Also, leaving it out now leaves the door open in the future to provide it. Whereas if we provide it now and later decide it was too much, taking it back is harder to do.
Please remove it, and then we can merge.
|
@gnapse I just pushed the requested changes. Basically, I removed
Please let me know if something else is needed 👍 |
hiwelo
changed the title
|
🎉 This PR is included in version 3.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
This pull request is adding
.toBeRequired()andallowing folks to test if a form element is.toBeOptional()methodsoptional orrequired.These methods are testing if the form element is having either
aria-requiredorrequiredattributes.In the case of a conflict between these two attributes, the value of
requiredis having precedence.Why:
This change is following the discussion within the issue #108.
How:
I added a new file in
src/with the different tests inspired fromto-be-disabled.js.I added the script to
src/index.js.I added a test in
src/__tests__/.Checklist: