Converts RichText Package to Typescript#69314
Converts RichText Package to Typescript#69314margolisj wants to merge 15 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
margolisj
changed the title
t-hamano
added
[Type] Code Quality
|
So I've tried to leave everything as 1 to 1 as humanly possible. Currently stuck at a few important type decisions. I'm going to add a review of where I'll need some opinion / guidance. I've also left TODO's in the code at places that are more gnarly. |
| end: number; | ||
| formats: Array< RichTextFormatList | undefined >; | ||
| replacements: Array< RichTextFormat | undefined >; | ||
| // TODO: Should these really be optional? |
There was a problem hiding this comment.
I believe this should be optional based on getting deeper into this problem.
packages/rich-text/src/types.ts
Outdated
| replacements: Array< RichTextFormat >; | ||
| start: number; | ||
| end: number; | ||
| formats: Array< RichTextFormatList | undefined >; |
There was a problem hiding this comment.
Are formats undefined outside of just the test cases? Same with the replacements below.
| */ | ||
| export function isFormatEqual( format1, format2 ) { | ||
| export function isFormatEqual( | ||
| format1: RichTextFormat | null, |
There was a problem hiding this comment.
Could be undefined instead of null.
| start, | ||
| end, | ||
| }: RichTextValue ): boolean | undefined { | ||
| }: { |
There was a problem hiding this comment.
Not sure if these types will pass when converting the components.
| ]; | ||
| const result = split( deepFreeze( record ), 6, 6 ); | ||
| // TODO: These were taking too many parameters | ||
| const result = split( deepFreeze( record ), 6 ); |
There was a problem hiding this comment.
Unsure about this parameter change.
| return doc.body.firstChild; | ||
| } | ||
|
|
||
| // function createElement( HTML: string ): HTMLElement { |
There was a problem hiding this comment.
Possibly want to swap to creating HTMLElements. JSDoc values were HTMLElements iirc yet using createNode.
|
Can we please update this branch from trunk, so that we can review it? |
3 participants
What?
Migrating the RichText package to Typescript. I'm working with and there are features that are not documented in the current types. I figured I might as well do the whole thing if allowed.
Why?
Type safety.
How?
Testing Instructions
Typecheck and unit tests.