-
Notifications
You must be signed in to change notification settings - Fork 219
Remove propTypes definitions from Top Rated Products block #9595
Remove propTypes definitions from Top Rated Products block #9595
Conversation
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 so much for working on this, @hritikchaudhary. I just added a comment regarding converting the file from a class component into a functional component.
I also noticed that this change introduces two TS errors in the following files:
assets/js/blocks/product-top-rated/block.tsx
assets/js/blocks/product-top-rated/index.js
The TS error in assets/js/blocks/product-top-rated/block.tsx
is related to import ServerSideRender from '@wordpress/server-side-render';
and can be ignored.
The TS error in assets/js/blocks/product-top-rated/index.js
is related to overloading the registerBlockType()
call. Could you look into this and also convert that file to TS?
/** | ||
* Component to handle edit mode of "Top Rated Products". | ||
*/ | ||
class ProductTopRatedBlock extends Component { | ||
|
||
class ProductTopRatedBlock extends Component< Props > { |
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.
Initially, I didn't notice that this file was still a class component. When working on files that are class components, we usually convert them to functional components. Do you think you could that in this case? If so, we should also adjust getInspectorControls()
, in the line below, accordingly.
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.
Hi sure I'll work on it... I was not aware about that.
Not sure how assets/js/blocks/product-top-rated/index.js
got flagged though, I didn't make any change in it, anyway will look into it.
Thanks for the feedback and direction!
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 so much for working on this, @hritikchaudhary. I just added a comment regarding converting the file from a class component into a functional component.
I also noticed that this change introduces two TS errors in the following files:
assets/js/blocks/product-top-rated/block.tsx
assets/js/blocks/product-top-rated/index.js
The TS error in assets/js/blocks/product-top-rated/block.tsx
is related to import ServerSideRender from '@wordpress/server-side-render';
and can be ignored.
The TS error in assets/js/blocks/product-top-rated/index.js
is related to overloading the registerBlockType()
call. Could you look into this and also convert that file to TS?
Hi @nielslange
|
Fantastic!
Your approach is absolutely fine.
Don't worry, @hritikchaudhary. Please take all the time that you need. After all, you are contributing in your spare time, which we appreciate very much. It's also great to hear that use this opportunity to learn React. That's fantastic. I'm sure you can find a lot of interesting code patterns within WooCommerce Blocks. |
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.
Thanks for applying the suggestions, @hritikchaudhary. The changes look great, and I'll merge this PR now. Thanks again for working on this. 🙏
Fixes #9519
Changes in the PR:
Removed propTypes definitions from product-search block and converted it to tsx.
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility