-
Notifications
You must be signed in to change notification settings - Fork 221
Product Tag Control to a Functional TypeScript component #10529
Conversation
The release ZIP for this PR is accessible via:
Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
Size Change: -164 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @mikejolley! Thank you for working on this PR! I left some questions below.
setLoading( false ); | ||
} ) | ||
.catch( () => { | ||
setLoading( false ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set the list
state in the previous code to an empty array inside the catch
block. Shouldn't we do the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided not to. It seemed like keeping a list visible on fail would be more graceful than showing an empty state with no items.
} | ||
onSearch( '' ); | ||
setIsMounted( true ); | ||
}, [ onSearch, isMounted ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a unique approach to executing useEffect
just once. Wouldn't providing an empty array achieve the same outcome? This way, we could eliminate the need for the isMounted
state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't work here because of the dependencies of the hook. Without the mounted check it would run on other renders if onSearch
changed (it did in testing). We've used this pattern before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification, @mikejolley! The code and testing look good! Let's 🚢 it.
Converts the Product Tag Control from a class based component with prop-types, to a Functional TypeScript component.
Fixes #9531
Screenshots
Testing
Automated Tests
Developer Testing
For debounced searches, this requires a store with many tags. You can short circuit this by editing the line:
To:
Then try searching for a tag. After a small 400ms debounce, confirm search results appear in the list.
User Facing Testing
Requires a store with several products which are assigned product tags.
WooCommerce Visibility