Rename toBeAccessible to toPassAxe

[gabalafou]

This pull request renames the matcher because it is potentially misleading. It includes a new section in the readme explaining why.

For background/motivation, refer to discussion in issue #21

[changeset-bot]

🦋 Changeset detected

Latest commit: dd2ab1e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
expect-axe-playwright	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[gabalafou]

done self-reviewing

[mskelton]

While I understand the reason why, the use case for testing a generated report is small, and I don't want to change the matcher name for the majority use case which is testing actual page elements. My recommendation would be to do this manually in your code like so:

expect.extend({ toHaveNoAxeViolations: matchers.toBeAccessible })

[mskelton]

Just occurred to me that it may be simpler and cleaner to add another matcher to this library toHaveNoAxeViolations which accepts an axe results object to keep the two matchers totally separate for each use case.

[gabalafou]

I fear I may not be communicating effectively because I'm doing too many things at once.

This PR isn't really about issue #21. It's related to that issue only tangentially.

The real issue here is described in the changes I made to the repo's readme via this PR. I would ask that you please look at the files changed in this PR and read what I added to the readme if you haven't yet.

I'm afraid that as a developer in the accessibility space, I have too often encountered other developers who think that accessibility is some kind of binary—as if you could take a page or an app and say yes, it's accessible. But in this respect, accessibility is much more like security. You wouldn't take a website, make it pass a bunch of security checks on it, and then say, yes, it's secure. In the same way, it's foolish to run a bunch of accessibility checks on a site, and then if all of them pass, to say, yes, it's accessible. That's my concern with the dev-facing API of this library, i.e., expect(page).toBeAccessible(). It re-enforces this already pervasive, completely erroneous notion in the minds of developers that you can ever accurately call something "accessible." You can't, not anymore than you can call something secure.

If I thought that most developers "get it" and it were only a tiny minority that have these mistaken assumptions, then I wouldn't think this were a big deal. But based on my and my colleagues' experiences, I don't think that's the case.

And if this didn't have the potential to negatively impact people who are already marginalized by society, then I could overlook it. But that is also not the case.

Ultimately, it's your decision to make, but I'm adding this comment because it wasn't clear to me from your previous comments whether I had effectively communicated what's really motivating this pull request. Thank you for listening and giving your time. :)

[mskelton]

Thanks for the explanation, I understand now why you want to rename the matcher and it makes good sense. I thought previously it was more to encapsulate the ability to pass a results object, so I now understand better your reason for this change.

I'm good with this change, but honestly I'd prefer to just make it a major change rather than deprecating the old matcher. That way it's just one and done and we can get rid of the deprecated code and make that text you added front and center in the README like jest-axe has.

Does that sound reasonable?

[gabalafou]

I so appreciate you being open to this change. 😊

If I understand you correctly, you're suggesting that I delete the toBeAccessible matcher and bump the version from 2 to 3, is that right?

[mskelton]

If I understand you correctly, you're suggesting that I delete the toBeAccessible matcher and bump the version from 2 to 3, is that right?

Yep, the version management is using changesets. So you'll need to add a changeset with a major version bump. The comment from the changeset bot should explain how that works.

[mskelton]

Also, my coworker mentioned that perhaps we could find a better name. toNotHaveAxeViolations is pretty verbose, and if combined with not then it's a double negative. Could we use a name like toPassAxe or something similar?

[gabalafou]

Haha I had the exact same thought regarding the double the negative. I then started imagining what the API might look like if it were a spellchecker, maybe something like:

expect(doc).toPassSpellchecker()

const results = runSpellchecker(doc)
expect(results).toPassSpellchecker()

So I think toPassAxe makes sense to me.

[mskelton]

The changelog should not be manually modified. Please follow the instructions here to add a changeset:

https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md#i-am-in-a-single-package-repository

[mskelton]

Looks to be some Prettier errors