-
Notifications
You must be signed in to change notification settings - Fork 218
Convert product-elements/price to TypeScript #7683
Convert product-elements/price to TypeScript #7683
Conversation
Co-authored-by: Manish Menaria <the.manish.menaria@gmail.com>
…hub.com:woocommerce/woocommerce-blocks into update/7092-convert-product-element-price-to-TS # Conflicts: # assets/js/atomic/blocks/product-elements/price/edit.tsx
…thub.com:woocommerce/woocommerce-blocks into update/7092-convert-product-elements-price-to-TS # Conflicts: # assets/js/atomic/blocks/product-elements/price/attributes.ts # assets/js/atomic/blocks/product-elements/price/edit.tsx # assets/js/atomic/blocks/product-elements/price/index.ts # checkstyle.xml
The release ZIP for this PR is accessible via:
|
TypeScript Errors ReportFiles with errors: 429
assets/js/atomic/blocks/product-elements/image/test/block.test.tsx
assets/js/base/components/formatted-monetary-amount/index.tsx assets/js/blocks/single-product/edit/layout-editor.tsx |
Size Change: +175 B (0%) Total Size: 1.01 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.
@wavvves and @imanish003 While solving a merge conflict, I accidentally closed #7534. Would you mind reviewing this updated PR again?
packages/prices/utils/price.ts
Outdated
@@ -73,7 +73,7 @@ export const getCurrencyFromPriceResponse = ( | |||
// Currency data object, for example an API response containing currency formatting data. | |||
currencyData?: | |||
| CurrencyResponse | |||
| Record< string, never > | |||
| Record< string, never | unknown > |
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 part had been changed to prevent a TS error related to the prices
attribute in the following section:
const currency = getCurrencyFromPriceResponse( prices );
…thub.com:woocommerce/woocommerce-blocks into update/7092-convert-product-elements-price-to-TS
productId?: number; | ||
align?: 'left' | 'center' | 'right'; |
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 part had been changed to prevent a TS error in the following section:
woocommerce-blocks/assets/js/blocks/cart/cart-cross-sells-product-list/cart-cross-sells-product.tsx
Line 51 in 04f3f4f
<ProductSaleBadge />
productId?: number; | ||
className?: string; | ||
textAlign?: 'left' | 'center' | 'right'; | ||
isDescendentOfQueryLoop?: boolean; |
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.
The attributes are optional to prevent a TS error in the following section:
woocommerce-blocks/assets/js/blocks/cart/cart-cross-sells-product-list/cart-cross-sells-product.tsx
Line 52 in 04f3f4f
<ProductPrice />
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
…to-TS # Conflicts: # assets/js/atomic/blocks/product-elements/price/attributes.ts # assets/js/atomic/blocks/product-elements/price/block.tsx # assets/js/atomic/blocks/product-elements/price/index.ts # assets/js/atomic/blocks/product-elements/sale-badge/types.ts # checkstyle.xml
The release ZIP for this PR is accessible via:
|
The release ZIP for this PR is accessible via:
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/base/components/product-price/index.tsx
|
…to-TS # Conflicts: # assets/js/blocks/single-product/block.tsx # assets/js/blocks/single-product/edit/layout-editor.tsx # assets/js/shared/context/product-data-context.tsx # checkstyle.xml
…thub.com:woocommerce/woocommerce-blocks into update/7092-convert-product-elements-price-to-TS
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.
Everything seems to be working properly
Thanks for your double-review, @wavvves! 🙌 |
We'll need to revise this, as some of this types are wrong. Too many arguments were made optional, but this causes the component to error out. For example: https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/285218ba2664af06c852a47918f7461d51b76c1f/assets/js/base/components/product-price/index.tsx#L160 You can see TypeScript complains and, in fact, if you try it, the component errors out. Should we add a new issue or reopen the original conversion issue? |
While solving a merge conflict, I accidentally closed #7534. This is the updated version that solved the previous merge conflict.
Fixes #7092
Currently, the
product-elements/price
is still written in JS and is using prop-types for type checking. This PR aims to convertproduct-elements/price
to TypeScript.Testing
User Facing Testing
Single Product block
to it.Product Price inner block
is still visible.