-
Notifications
You must be signed in to change notification settings - Fork 467
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
feat: upgrade aria-query to 5.3.0 #1241
feat: upgrade aria-query to 5.3.0 #1241
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 69d966a:
|
Codecov Report
@@ Coverage Diff @@
## alpha #1241 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 1038 1041 +3
Branches 349 349
=========================================
+ Hits 1038 1041 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @jlp-craigmorten! |
Appreciate it - I’ve just raised #1242 r.e. the CI issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great, thanks :)
I left a few comments.
848b2be
to
d89d507
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @MatanBobi good for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. Thanks a lot for the work :)
@eps1lon do you think we should make this a breaking change?
My understanding of SemVer is that we only need to publish breaking changes in a major release if we upgrade to a version of a spec that's no longer back compatible with a version that we've documented supporting. Here are the specification versions we link to, organized by our APIs: |
Thanks @nickmccurdy! |
This is now blocked behind #1242 (as is any other changes to this repo) |
I agree with @timdeschryver about this being a breaking change that should cause a major version release. |
@MatanBobi that would mean to drop support for Node v14, right? |
Yes, definitely. In this major we'll also drop support for node 14 with these changes. @eps1lon do you have any concerns? |
Hi @jlp-craigmorten :) Thanks again for the effort you've put into this one! |
I was referring to the docs, I've added links above to give more context. That being said, it seems like we've already decided on a major version, which I support. |
If you merge in from |
Thanks @nickmccurdy, we'll probably need to update the places where we direct to 1.1 as part of the major release :) |
* chore: add accessibility-alt-text-bot * fix formatting issues
Just done a scan through the aria-query changeset from
Feel free to condense / summarise as feel is best! There is one other file change in this PR in addition to the changeset from upgrade For the footer and header element roles, the "scoped to body element" constraint isn't honoured by Update: so for the header element it is |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Since we're following the spec with this one, I think it's safe to at least merge to |
* feat: upgrade aria-query to 5.3.0 * fix: correctly handle img with empty alt * feat: pin `aria-query` version * chore: add accessibility-alt-text-bot (testing-library#1240) * chore: add accessibility-alt-text-bot * fix formatting issues * test: add coverage for footer to confirm it's expected implicit role as contentinfo --------- Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com> Co-authored-by: Matan Borenkraout <Matanbobi@gmail.com>
upgrades `@testing-library/react` from 14 -> 16. upgrades `@testing-library/dom` from 9 -> 10 Notes: - @testing-library/dom is now a peer dependency, have to install - Support for react 19 - Upgrades aria-query to 5.3.0 testing-library/dom-testing-library#1241 - changes getByRole('link') `a element now requires a set href attribute to have an implicit link role`
What: Upgrade
aria-query
to version 5.3.0.Why: Receive latest specification updates for role / element mappings to align with WAI ARIA 1.2.
Fixes #1100
Fixes #1150
Fixes #1201
Fixes #1234
Fixes #1239
How: Upgrading version in
package.json
. Updating tests to mirror changes.Checklist:
docs site
There are a couple changes of note as a result of this upgrade, namely:
menuitem
element no longer has associated roles. This also removes the only example of an element with two role mappings.p
andem
now have dedicated roles (paragraph
andemphasis
respectively) inline with HTML-ARIA / WAI-ARIA 1.2. Associated tests have been updated / modified to suit.generic
role also results in some changes. E.g. anchor tags with nohref
are nowgeneric
under HTML-ARIA.There are several other role / element mapping changes as a result of this change to align with specifications.
Given the number of role additions, removals, and modifications (including changes to role and/or changes to the constraints for a particular role) this should be considered a breaking change.