Added strict typings for ARIAMixin with literals#1840
Added strict typings for ARIAMixin with literals#1840DeepDoge wants to merge 5 commits intomicrosoft:mainfrom
ARIAMixin with literals#1840Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
|
@microsoft-github-policy-service agree |
…nd `ariaBrailleRoleDescription`
saschanaz
left a comment
saschanaz
left a comment
There was a problem hiding this comment.
This needs some unit test, take a look here: https://github.com/microsoft/TypeScript-DOM-lib-generator/blob/main/unittests/files/autocomplete.ts
inputfiles/overridingTypes.jsonc
Outdated
| "properties": { | ||
| "property": { | ||
| "ariaColCount": { | ||
| "overrideType": "`${bigint}` | \"\"" |
There was a problem hiding this comment.
Does bigint have any difference than number when used here? Quickly tried but number doesn't reject huge number either.
There was a problem hiding this comment.
${bigint} only allows integers, ${number} also allows floats.
So based on what w3c says, I used ${bigint} for integer types and ${number} for number types.
Also I allowed empty string "" on these. Thinking people might use empty string instead of null.
But now since I'm also removing string & {} from enums, making them stricter. Should I also remove the empty string ""?
inputfiles/overridingTypes.jsonc
Outdated
| // "overrideType": "" | ||
| // } | ||
| "role": { | ||
| "overrideType": "\"toolbar\" | \"tooltip\" | \"feed\" | \"math\" | \"presentation\" | \"none\" | \"note\" | \"application\" | \"article\" | \"cell\" | \"columnheader\" | \"definition\" | \"directory\" | \"document\" | \"figure\" | \"group\" | \"heading\" | \"img\" | \"list\" | \"listitem\" | \"meter\" | \"row\" | \"rowgroup\" | \"rowheader\" | \"separator\" | \"table\" | \"term\" | \"associationlist\" | \"associationlistitemkey\" | \"associationlistitemvalue\" | \"blockquote\" | \"caption\" | \"code\" | \"deletion\" | \"emphasis\" | \"insertion\" | \"paragraph\" | \"strong\" | \"subscript\" | \"superscript\" | \"time\" | \"scrollbar\" | \"searchbox\" | \"separator\" | \"slider\" | \"spinbutton\" | \"switch\" | \"tab\" | \"tabpanel\" | \"treeitem\" | \"combobox\" | \"menu\" | \"menubar\" | \"tablist\" | \"tree\" | \"treegrid\" | \"banner\" | \"complementary\" | \"contentinfo\" | \"form\" | \"main\" | \"navigation\" | \"region\" | \"search\" | \"alert\" | \"log\" | \"marquee\" | \"status\" | \"timer\" | \"alertdialog\" | \"dialog\" | (string & {})" |
There was a problem hiding this comment.
Where did you get this list? Have you checked everything here has multiple implementation support?
- Remove commented items
- Remove `string & {}` from types
- Remove empty strings from number types
- Update and export roles as enum `ARIARole`
- Add type for `ariaInvalid`
- Add unit tests
|
Removed commented items. Removed |
|
@saschanaz Should I include roles listed at https://www.w3.org/TR/dpub-aria-1.1/#role_definitions? Svelte includes them in |
|
Stumbled across this PR - seems like a great addition! @saschanaz Anything you need before this can be merged? @DeepDoge I wouldn't include the dpub roles as they are only a proposed recommendation and are not fully baked. |
3 participants
Added stricter typings for
ARIAMixinwith a fallback to also allow any other string. Also used${bigint}and${number}to type, integer and number only properties.I wasn't sure if I should create a new
enumtype for each ARIA property, so I inlined them in theoverrideType.Fixes the issue: #1835