Update/refactor url list #27559

jhnstn · 2020-12-07T19:38:32Z

Description

Part of #22890. Refactor the URLInput component to a functional component using hooks.

How has this been tested?

Manually tested to make sure URL input behavior is consistent with existing behavior using #24089 as a guide.

Screenshots

Types of changes

Refactoring to use hooks.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

See #27489

packages/block-editor/src/components/url-input/index.js

Also collect similar value state concerns

ntsekouras

Thanks for working on this! It's a pretty complex component :)

I have checked most of the functions but needs more checking in lifecycle components. Let's address some of the comments and review again.

packages/block-editor/src/components/url-input/index.js

Removes the need for parentOnChange

This allows other components to mock when is not directly under testing

jhnstn · 2021-01-12T21:00:33Z

hi @ntsekouras , thanks again for all the feedback. I got pulled away from this issue but I'm hoping to get back on it this week. I've pushed updates per your comments ( which were super helpful ) and I believe this is ready for a second look. I see a few e2e tests failing but they seem unrelated to these changes, I'd re-run to verify it's just a blinking test but I don't have access to do that yet. Thanks again.

packages/block-editor/src/components/url-input/index.js

ntsekouras · 2021-01-14T11:28:04Z

packages/block-editor/src/components/url-input/index.js

-			! this.isUpdatingSuggestions &&
-			! ( suggestions && suggestions.length )
+			! isUpdatingSuggestions() &&
+			! suggestions.length


Suggested change

! suggestions.length

! suggestions?.length

Not sure this guard is needed. suggestions is initialized to an empty array in a useState call. There is one call to setSuggestions with the result from the suggestions fetch but there is an truthy check before it's called. I'll add an Array.isArray check on the result (length won't work here since we want to update suggestions if they are empty)

Added Array check in d5fe5e8

It's okay with the check you added, but actually Array.isArray catches null and undefined, so you need just this check without checking the plain result.

The conditional is something like result && Array.isArray( result ) which should prevent the setSuggestions call with null or undefined. But I thought Array.isArray will return false for null or undefined ? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#using_array.isarray

I'm not sure I understood correctly but my comment was about result && Array.isArray( result ) being equivalent to Array.isArray(result)

ah I see, I total misread your comment @ntsekouras , my bad.

packages/block-editor/src/components/url-input/index.js

ntsekouras

Great work here @jhnstn! Thanks for refactoring this quite complicated component. 💯

I have left some small notes but this feels to be in good place. By testing I noticed a different behavior with the master, which also feels like it should behave like it does here :) Other than that it LGTM.

The first part of the gif is from your branch and when I clear the search term it shows the initial results. In master as you can see it doesn't (second part of gif).

It would be great for someone more experienced in class components and especially getDerivedStateFromProps part to take a look as well. @ellatrix if you have some time, I know you have done tons of such refactorings. This includes my two comments about wheter a useEffect should be used.

ntsekouras · 2021-01-14T13:13:06Z

packages/block-editor/src/components/url-input/index.js

+		[]
+	);
+
+	if ( prevInstanceId !== instanceId ) {


Should this be in a useEffect? 🤔

Good question, tbh I'm not sure since this logic is from the existing getDerivedStateFromProps. I tried following back through the commit history to see why getDerivedStateFromProps was being used but nothing jumped out. Then after some research on how to refactor getDerivedStateFromProps to use hooks I noticed that folks are just comparing refs to state outside of useEffect.

packages/block-editor/src/components/url-input/index.js

ntsekouras · 2021-01-14T13:19:12Z

packages/block-editor/src/components/url-input/index.js

-	componentWillUnmount() {
-		delete this.suggestionsRequest;
-	}
+		if ( showInitialSuggestionsChanged ) {


Should this be in a useEffect as well? 🤔

jhnstn · 2021-01-14T17:42:51Z

Thanks again @ntsekouras !

The first part of the gif is from your branch and when I clear the search term it shows the initial results. In master as you can see it doesn't (second part of gif).

Interesting, I had scrutinized that behavior and I was fairly confident that I was seeing the same thing on master. Regardless I'll get those to match.

packages/block-editor/src/components/url-input/index.js

jhnstn · 2021-04-05T17:07:27Z

Closing due to the changes going stale. Resolving the conflicts will likely take longer than starting a new PR

jhnstn added 2 commits December 7, 2020 11:29

Refactor to functional component

88ac273

Add back rebase conflict

39ea2b8

See #27489

jhnstn requested a review from ellatrix as a code owner December 7, 2020 19:38

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Dec 7, 2020

skorasaurus added the [Type] Code Quality Issues or PRs that relate to code quality label Dec 10, 2020

Merge branch 'master' into update/refactor-url-list

2638796

jhnstn marked this pull request as draft December 10, 2020 18:12

youknowriad requested a review from a team December 10, 2020 18:20

jhnstn added 2 commits December 10, 2020 15:26

render 'false' for dom node property when null

b2d3f90

Fix tests

bc06c24

jhnstn marked this pull request as ready for review December 11, 2020 20:46

ntsekouras reviewed Dec 15, 2020

View reviewed changes

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved

Add dependency to useEffect

f41b9e2

Also collect similar value state concerns

ntsekouras reviewed Dec 16, 2020

View reviewed changes

jhnstn added 4 commits December 16, 2020 09:53

move onChange to inline function on inputProps

e8562aa

Removes the need for parentOnChange

Switch withSelect to useSelect

63754f2

Update default and experimental rendering functions

e8e07b6

Reduce lodash usage

f6a9403

jhnstn marked this pull request as draft December 17, 2020 03:51

jhnstn added 5 commits December 18, 2020 13:34

Put default in useSelect destructure

232fe1e

This allows other components to mock when is not directly under testing

Rename loading to isLoading

792b8ef

Replace withInstanceId with useInstanceId

ccfa857

Fix destructure return

f2ef50a

Clean up effects and getDerivedStateFromProps refactor

97c3343

jhnstn marked this pull request as ready for review January 12, 2021 21:01

ntsekouras reviewed Jan 14, 2021

View reviewed changes

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jan 14, 2021

View reviewed changes

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jan 14, 2021

View reviewed changes

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jan 14, 2021

View reviewed changes

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jan 14, 2021

View reviewed changes

jhnstn added 5 commits January 14, 2021 13:55

Merge branch 'master' into update/refactor-url-list

94d7671

Fix smaller issues

bd7c882

Make sure initial suggestions only appear per prop setting

2892b8d

Only update suggestions if results are an array

d5fe5e8

Go back to default __experimentalFetchLinkSuggestions and add comment

03c4c2b

jhnstn commented Jan 15, 2021

View reviewed changes

packages/block-editor/src/components/url-input/index.js Show resolved Hide resolved

Remove redundant result check

88882df

Base automatically changed from master to trunk March 1, 2021 15:44

jhnstn closed this Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/refactor url list #27559

Update/refactor url list #27559

jhnstn commented Dec 7, 2020 •

edited

Loading

ntsekouras left a comment

jhnstn commented Jan 12, 2021 •

edited

Loading

ntsekouras Jan 14, 2021

jhnstn Jan 14, 2021

jhnstn Jan 15, 2021

ntsekouras Jan 15, 2021 •

edited

Loading

jhnstn Jan 15, 2021

ntsekouras Jan 16, 2021

jhnstn Jan 18, 2021

ntsekouras left a comment

ntsekouras Jan 14, 2021

jhnstn Jan 14, 2021 •

edited

Loading

ntsekouras Jan 14, 2021

jhnstn commented Jan 14, 2021

jhnstn commented Apr 5, 2021

Update/refactor url list #27559

Update/refactor url list #27559

Conversation

jhnstn commented Dec 7, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ntsekouras left a comment

Choose a reason for hiding this comment

jhnstn commented Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnstn Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnstn commented Jan 14, 2021

jhnstn commented Apr 5, 2021

jhnstn commented Dec 7, 2020 •

edited

Loading

jhnstn commented Jan 12, 2021 •

edited

Loading

ntsekouras Jan 15, 2021 •

edited

Loading

jhnstn Jan 14, 2021 •

edited

Loading