-
Notifications
You must be signed in to change notification settings - Fork 12
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
Convert HighlightOverlay to TypeScript #1375
Comments
Conversion is done, I did some fuzz testing and basic spot checking and all seems well. The part I want to review in particular is scenery/js/accessibility/voicing/InteractiveHighlighting.ts Lines 31 to 52 in 86fc973
@zepumph or @jonathanolson does this seem right to you or is there a way to reduce the duplication of InteractiveHighlightingInterface? |
Discussed with @jonathanolson , he recommended looking at the WidthSizableNode example to avoid the duplication. // Some typescript gymnastics to provide a user-defined type guard that treats something as widthSizable
const wrapper = () => WidthSizable( Node );
type WidthSizableNode = InstanceType<ReturnType<typeof wrapper>>;
const isWidthSizable = ( node: Node ): node is WidthSizableNode => {
return node.widthSizable;
}; |
#1375 (comment) is working great for both InteractiveHighlighting and ReadingBlock. That is it for the TS conversion of HighlightOverlay. Closing. |
It would be nice to have types for highlights in #1372 so I took a stab at converting HighlightOverlay to TypeScript. But I would like to review a portion of the conversion with someone that is familiar with the TypeScript mixin/trait pattern.
Most notably I needed typing for "A node that mixes ReadingBlock/InteractiveHighlighting". I looked at Poolable and I think I did it in a similar way. It seems to work but duplicates much of the interface of ReadingBlock and InteractiveHighlighting to define a type.
Also, I had to use
IProperty<boolean>
instead ofBooleanProperty
for theHIghlightOverlayOptions
so that those Properties work well when they are set from the Properties of FocusManager in Display.ts. Is there a reason to useIProperty<boolean>
instead ofBooleanProperty
?Commits coming.
The text was updated successfully, but these errors were encountered: