Rewrite <FormTokenField> to functional component and Typescript.
#41216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -695 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
torounit
changed the title
<TokenInput> in FormTokenField to functional component and Typescript.<FormTokenField> to functional component and Typescript.
torounit
changed the title
<FormTokenField> to functional component and Typescript.<FormTokenField> to functional component and Typescript.
torounit
added
[Type] Code Quality
mirka
reviewed
May 27, 2022
There was a problem hiding this comment.
Wow, thank you for taking on this monster component! 👏
I've looked through all but the main index.tsx file and suggestions-list.tsx. Before I go any further, would you be able to move the bug fix that you did to a separate PR? This is a complex component written a long time ago and we're very unfamiliar with it. We'd appreciate any help from your end so we can review it safely with confidence.
That means:
- Extracting any functional changes that aren't part of the essential refactor, and move them to a separate PR so we can concentrate on reviewing the refactor.
- Adding GitHub inline comments to any notable changes that needed to happen. (For example, things like the removal of the TODO comment and
withSafeTimeoutin suggestions-list. I can't tell why these changes were needed, so it'd be very helpful if you could add some explanation.)
Thank you so much! 🙌
| onlyScrollIfNeeded?: boolean; | ||
| } | ||
|
|
||
| declare module 'dom-scroll-into-view' { |
There was a problem hiding this comment.
Thanks for adding the types for this 😅 I see the package is unmaintained and also doesn't have an @types!
I guess we're waiting on Safari now to implement the "nearest" options so we can replace it with Element.scrollIntoView()? Maybe by next year!
| @@ -10,7 +10,6 @@ import ReactDOM from 'react-dom'; | |||
| */ | |||
There was a problem hiding this comment.
We'll want to update these tests to use React Testing Library eventually, but I guess it's nice to have these act wrappers in the meantime 😄
Comment on lines
63
to
67
| __experimentalValidateInput={ | ||
| __experimentalValidateInput | ||
| ? ( newValue ) => continents.includes( newValue ) | ||
| : () => true | ||
| } |
There was a problem hiding this comment.
I was going back and forth on whether we should do this in the Storybook (is it too confusing?), but ultimately I think it's probably good. I think it will be clearer once we have the prop descriptions showing up in the props table.
There was a problem hiding this comment.
In Storybook, the type shown is token: string) => boolean but the control says "Set boolean" — wouldn't that be a bit confusing?
There was a problem hiding this comment.
Updated storybook and type documents. How is it?
packages/components/src/form-token-field/test/lib/token-field-wrapper.js
Outdated
Show resolved
Hide resolved
torounit
commented
May 27, 2022
torounit
commented
May 27, 2022
torounit
commented
May 27, 2022
Comment on lines
454
to
481
| const selected = _value.map( ( item ) => { | ||
| if ( typeof item === 'string' ) { | ||
| return item; | ||
| } | ||
| return item.value; | ||
| } ); |
There was a problem hiding this comment.
Fixed a problem where a filter in the query loop block would suggest something that was already selected.
Separating to another PR would result in a type mismatch.
mirka
reviewed
May 30, 2022
There was a problem hiding this comment.
Awesome, thank you! I haven't gotten to suggestions-list yet, but I've reviewed the main file. I'll probably be able to review the suggestions-list later today or maybe tomorrow.
Before I forget, could you also add a changelog entry please? This probably warrants two entries — one under Bug Fix and another under Internal 🌟
Comment on lines
117
to
118
| updateSuggestions( suggestionsDidUpdate ); | ||
| }, [ suggestions ] ); |
There was a problem hiding this comment.
In the original logic, updateSuggestions() also runs on value updates, presumably because getMatchingSuggestions() depends on value. Was this logic removed on purpose?
gutenberg/packages/components/src/form-token-field/index.js
Lines 86 to 88 in 046ca65
There was a problem hiding this comment.
I think this is a mistake. I will fix it.
Comment on lines
120
to
133
| useEffect( () => { | ||
| updateSuggestions(); | ||
| }, [ incompleteTokenValue ] ); |
There was a problem hiding this comment.
Note for posterity — This useEffect mimics the setState callback in the original logic here:
gutenberg/packages/components/src/form-token-field/index.js
Lines 244 to 247 in 046ca65
| ); | ||
|
|
||
| updateSuggestions( suggestionsDidUpdate ); | ||
| }, [ suggestions ] ); |
There was a problem hiding this comment.
We are in the process of reenabling the react-hooks/exhaustive-deps eslint rule (#41166), and this deps array here is going to trigger a warning:
React Hook useEffect has missing dependencies: 'prevSuggestions' and 'updateSuggestions'. Either include them or remove the dependency array.
And speaking of updateSuggestions... this and getMatchingSuggestions are really hard to verify correctness because they rely on so much outer state ☠️ Ideally I'd want to refactor them into pure-ish functions and move them out of the main component function! But, since this PR is already pretty big, we should probably keep changes to a minimum. How about we do something like this for now?
Suggested change
| }, [ suggestions ] ); | |
| // TODO: updateSuggestions() should first be refactored so its actual deps are clearer | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [ suggestions, prevSuggestions ] ); |
mirka
reviewed
May 31, 2022
Closed
* Remove findRenderedComponentWithType as it does not work with functional components.
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
torounit
force-pushed
the
refactor/components/form-token-field
branch
from
af8adec
to
e1bc853
Compare
mirka
reviewed
Jun 2, 2022
Comment on lines
63
to
67
| __experimentalValidateInput={ | ||
| __experimentalValidateInput | ||
| ? ( newValue ) => continents.includes( newValue ) | ||
| : () => true | ||
| } |
mirka
approved these changes
Jun 2, 2022
| * this is needed to remove leading and trailing spaces from tag names, like wp-admin does. | ||
| * Otherwise the REST API won't save them.) | ||
| * | ||
| * @default ( token: string ) => token.trim() |
Co-authored-by: Lena Morita <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref: #35744, #22890
What?
Rewrites the component JavaScript in packages/components/form-token-field in TypeScript and refactor to functional component.
Also, fixed an issue where the filter in the query loop block would suggest something that was already selected.
Part of #35744
Testing Instructions