-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Adds semantics role checks #162290
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
Adds semantics role checks #162290
Conversation
232fabc to
8ea2a7d
Compare
hannah-hyj
left a comment
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.
LGTM with some comments on tests
| expect(SemanticsRole.values.toSet(), DebugSemanticsRoleChecks.kChecks.keys.toSet()); | ||
| }); | ||
|
|
||
| group('tab', () { |
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.
add a successful case too?
| child: Semantics(role: SemanticsRole.tab, child: const Text('a tab')), | ||
| ), | ||
| ); | ||
| expect(tester.takeException(), isFlutterError); |
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.
can you also expect the error string here, so it will be more clear
| }); | ||
| }); | ||
|
|
||
| group('tabBar', () { |
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.
ditto, add a successful case too
| /// to [kChecks]. | ||
| @visibleForTesting | ||
| class DebugSemanticsRoleChecks { | ||
| DebugSemanticsRoleChecks._(); |
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.
Drive-by comment: this can be a final class ? Then DebugSemanticsRoleChecks._(); is not needed.
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.
final class can still be instantiated I think? I use sealed instead
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.
Yeah, you're right, I had to double check the class modifiers reference :)
But yeah, we should be able to eliminate the constructor anyway
b5b9232 to
956fb4a
Compare
956fb4a to
24ebf55
Compare
|
autosubmit label was removed for flutter/flutter/162290, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Adds test and error detection system for semantics roles.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.