-
Notifications
You must be signed in to change notification settings - Fork 293
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
Update @material/web
version and GM3 Checkbox component.
#7108
Conversation
The @material/web md-checkbox will now automatically get role="presentation" applied.
Note that the tests for clicking on the label have been disabled due to incompatibilities between the latest @material/web version and JSDom.
new Event( 'change', { bubbles: true, cancelable: true } ) | ||
); | ||
}; | ||
const labelID = `${ id }-label`; |
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.
Noting that the element-internals-polyfill
package (introduced in this PR to to polyfill attachInternals()
for JSDom) sets aria-labelledby
reminded me that we should do that ourselves for non-polyfilled production scenarios.
Build files for abb85cc have been deleted. |
Size Change: +274 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
@@ -115,8 +102,8 @@ export default function Checkbox( { | |||
<md-checkbox | |||
id={ id } | |||
ref={ ref } | |||
role="checkbox" |
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.
The md-checkbox
now has its role automatically set to "production"
, overriding what we specify here, so there's no point setting it.
).toBe( 'Complex Label With 5 Sub Children' ); | ||
} ); | ||
|
||
it( 'should attach the onKeyDown handler when present', () => { | ||
const onKeyDown = jest.fn(); | ||
|
||
const { getByRole } = render( | ||
const { getByLabelText } = render( |
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.
Use getByLabelText()
rather than getByRole()
to reflect the fact we're no longer setting the role directly.
@@ -34,6 +38,7 @@ module.exports = { | |||
'<rootDir>/node_modules', | |||
'<rootDir>/build', | |||
], | |||
modulePathIgnorePatterns: [ '<rootDir>/.vscode' ], |
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.
When using the GitHub Copilot plugin, it updates local VSCode settings with regularity when switching back to VSCode, causing watched tests to re-run without the source files changing. Adding this path to modulePathIgnorePatterns
prevents this from happening.
Summary
Addresses issue:
@material/web
version and GM3Checkbox
component. #6696Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist