Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toBeReadonly matcher #241

Closed
wants to merge 4 commits into from
Closed

Add toBeReadonly matcher #241

wants to merge 4 commits into from

Conversation

rubenmoya
Copy link
Contributor

@rubenmoya rubenmoya commented May 6, 2020

What: This PR adds a toBeReadonly matcher.

Why: Today I was testing some inputs on a project I'm working on and I thought this would be useful instead of having to use toHaveAttribute.

How: I checked how the other matches worked, I've searched on MDN what inputs support readonly and which aria roles accept aria-readonly (although I'm not sure I did it right).

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

I'm not sure if it should be toBeReadOnly or toBeReadonly. Also I'd love some help with the aria-roles, I'm not sure if I did it right to be honest.

There is a check about type definitions, where can I find them?

@rubenmoya rubenmoya changed the title Pr/add readonly matcher Add toBeReadonly matcher May 6, 2020
@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #241   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        21    +1     
  Lines          268       279   +11     
  Branches        65        68    +3     
=========================================
+ Hits           268       279   +11     
Impacted Files Coverage Δ
src/to-be-readonly.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2afc2c5...be8fbdd. Read the comment docs.

@gnapse
Copy link
Member

gnapse commented May 7, 2020

There's one thing here I am not fully convinced of, and I would have rather had you opening an issue before hand to discuss, but here it is:

We previously considered adding aria-disabled to the criteria covered by the toBeDisabled, and we decided not to do it, because unlike the more traditional and stricter disabled attribute, aria-disabled does not actually disable anything. See #144 for the full discussion.

I am worried that this poses a similar issue: that readonly actually makes a control read-only (i.e. users can't type) but the presence of aria-readonly does not have the same effect. Is that correct?

If so, then I think we are in the same situation as in #144 with the disabled vs. aria-disabled attributes, and maybe we should support only the readonly.

Other than that I had already reviewed your PR and looked good, but I wanted to take this comment out up front because if I'm right, it kinda changes a lot.

@gnapse
Copy link
Member

gnapse commented May 7, 2020

Actually, I totally forgot, but we already had an attempt to contribute a .toBeReadonly in #204 and we did not accept it. Let me know what you think about the discussion there.

@rubenmoya
Copy link
Contributor Author

Thank you for your response, and sorry for getting carried away and opening the PR before filling an issue 😅

After reading #144 and #204 I think you're right, if we don't check for aria-readonly it would be a small syntactic sugar for toHaveAttribute and I understand that we don't want to expand the API surface, so let's close this.

At least it was a good exercise to get to know how the project works 😂

Thank you for your time!

@rubenmoya rubenmoya closed this May 7, 2020
@rubenmoya rubenmoya deleted the pr/add-readonly-matcher branch May 7, 2020 18:47
@gnapse
Copy link
Member

gnapse commented May 7, 2020

Thank you for your time. And yes, that's why it's almost always a good idea to propose new features before implementing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants