fix(types): update accessibility types to match React's #1087
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
| } | ||
|
|
||
| export interface AccessibilityAttributes extends AriaWidgetAttributes, AriaRelationshipAttributes { | ||
| role?: AriaRole | ||
| tabIndex?: string | ||
| tabIndex?: number |
There was a problem hiding this comment.
tabIndex is number even in TS definitions: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts#L7779
| 'aria-haspopup'?: string | ||
| 'aria-hidden'?: string | boolean | ||
| 'aria-invalid'?: string | ||
| 'aria-autocomplete'?: 'none' | 'inline' | 'list' | 'both' |
There was a problem hiding this comment.
could you, please, point to the React type that this prop's type has been taken from? Asking just because it would be much better if this kind of relationship (React prop's type for accessibility's type) would be explicit
There was a problem hiding this comment.
HTMLAttributes: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L1475
Do you want to leave there a comment with this type?
There was a problem hiding this comment.
@kuzhelov by explicit, do you mean a compile time dependency on React? Please note that behaviors should be plain javascript (or typescript) theoretically usable with other frameworks as well.
There was a problem hiding this comment.
@jurokapsiar, yes, exactly - I would expect that there would be this dependency expressed explicitly.
Please note that behaviors should be plain javascript (or typescript) theoretically usable with other frameworks as well.
While I do understand that we need this to be framework-agnostic, it seems a bit misleading that we are trying to generalize types based on React ones. The thing that I am worrying is that now what we are doing, essentially, is introducing this dependency you are talking about, with the only difference that it is implicit, compared to the approach I've suggested before.
Ideally, would expect some sort of common types library (TS lib.dom.d.ts?) for HTML attributes that we could rely on, without having React as a dependency
There was a problem hiding this comment.
that we are trying to generalize types based on React ones.
But React types are correct in this case...
…x/acc-types # Conflicts: # CHANGELOG.md
Codecov Report
@@ Coverage Diff @@
## master #1087 +/- ##
==========================================
+ Coverage 81.76% 81.76% +<.01%
==========================================
Files 701 701
Lines 8570 8573 +3
Branches 1171 1245 +74
==========================================
+ Hits 7007 7010 +3
Misses 1548 1548
Partials 15 15
Continue to review full report at Codecov.
|
This PR fixes accessibility types.