Repository navigation
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.
| 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? |
|
This PR needs to be updated with the latest changes from trunk. @margolisj, could you please take care of it? |
|
@im3dabasia @manzoorwanijk Are people taking merging TS in PRs to gutenberg seriously now? I will have to check with my current employee and go through the standard process for that so please let me know, and if this will actually get merged, I'll get approval and clean this up. |
Let me be honest here. This PR got stale just because I didn't feel confident reviewing it alone without the people who work with the Rich Text package, because it's too big a change for a single PR. Let us make sure this gets through. So, here is what we can do:
Once ready, please ping me, and I would be happy to help ship it. Some important feedback for those smaller PRs:
|
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.