-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
fix(eslint-plugin-template): accessibility-valid-aria not reporting i… #278
fix(eslint-plugin-template): accessibility-valid-aria not reporting i… #278
Conversation
…nvalid 'aria-' values
// As in the time it being write the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) | ||
// is incorrect (`value` should be `values`), we created this hackish type to avoid `any`. | ||
type ARIAPropertyDefinitionHack = Omit<ARIAPropertyDefinition, 'value'> & { | ||
readonly values: ARIAPropertyDefinition['value']; | ||
}; |
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.
I hope the comment is clear enough to express the intent.
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.
Thanks yes, made sense, just added some minor English language suggestions
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.
I've opened a PR in the DefinitelyTyped repo: DefinitelyTyped/DefinitelyTyped#50528. What do you think about delaying this merge a little bit until the PR is merged? Of course, if it takes too long to merge, we can move on as is.
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.
I removed this as DefinitelyTyped/DefinitelyTyped#50528 has been merged.
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.
Just the minor suggestions to apply, thanks a lot!
|
||
import { | ||
createESLintRule, | ||
getTemplateParserServices, | ||
} from '../utils/create-eslint-rule'; | ||
|
||
// As in the time it being write the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) |
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.
// As in the time it being write the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) | |
// At the time of writing, the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) |
|
||
import { | ||
createESLintRule, | ||
getTemplateParserServices, | ||
} from '../utils/create-eslint-rule'; | ||
|
||
// As in the time it being write the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) | ||
// is incorrect (`value` should be `values`), we created this hackish type to avoid `any`. |
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.
// is incorrect (`value` should be `values`), we created this hackish type to avoid `any`. | |
// is incorrect (`value` should be `values`), we created this hacky workaround type to avoid `any`. |
// As in the time it being write the `ARIAPropertyDefinition` (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/64ae44025dbd41afb3b865ca37deddef7de1c5fb/types/aria-query/index.d.ts#L302-L306) | ||
// is incorrect (`value` should be `values`), we created this hackish type to avoid `any`. | ||
type ARIAPropertyDefinitionHack = Omit<ARIAPropertyDefinition, 'value'> & { | ||
readonly values: ARIAPropertyDefinition['value']; | ||
}; |
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.
Thanks yes, made sense, just added some minor English language suggestions
…rting invalid 'aria-' values
…s95/angular-eslint into fix/accessibility-valid-aria-value
…o fix/accessibility-valid-aria-value
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.
Thanks again!
…nvalid 'aria-' values
Relates to #272 (comment).
PS: Not sure it should be considered a feat or a fix.