-
Notifications
You must be signed in to change notification settings - Fork 218
Conversation
Size Change: +2.72 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
bf7f997
to
8a16e36
Compare
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 tested cart and checkout block (particularly shipping) and noticed no regressions 👍🏻
I have added a few comments, and some issues I spotted in TS console.
Other than that I also have 2 questions which may not require action, but we should add to our hack.md file as notes.
- Should we rename all
.js
to.ts
in packages we convert? I noticed inshipping-rates-control
the index file was left as a JS file. - When do we use type-defs vs a types.ts file within the package/component directory? I think we may all be handling this is different ways. Is it just based on if they are shared? Personally I prefer types.ts so it's in the same context as the component using it.
className="wc-block-components-radio-control__description" | ||
> | ||
{ description } | ||
</span> | ||
) } | ||
{ secondaryDescription && ( | ||
<span | ||
id={ id ? `${ id }__secondary-description` : null } | ||
id={ id && `${ id }__secondary-description` } |
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.
👍🏻 good improvement, not sure why null was there originally.
@@ -64,7 +74,7 @@ const FormattedMonetaryAmount = ( { | |||
className | |||
); | |||
const numberFormatProps = { | |||
displayType, | |||
displayType: 'text' as NumberFormatProps[ 'displayType' ], |
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 think we should move this one to our FormattedMonetaryAmount
props since we do use this in other places and we always set the default value to 'text'. The type can then go within FormattedMonetaryAmountProps
.
/** | ||
* Formatted price component. | ||
* | ||
* Takes a price and returns a formatted price using the NumberFormat component. | ||
* | ||
* @param {Object} props Component props. | ||
* @param {string=} props.className CSS class used. | ||
* @param {string} props.className CSS class used. |
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 can remove this docblock and inline the comments now (within FormattedMonetaryAmountProps
)
import { getCurrencyFromPriceResponse } from '@woocommerce/price-format'; | ||
import FormattedMonetaryAmount from '@woocommerce/base-components/formatted-monetary-amount'; |
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 one gives me an error. Casing of @woocommerce/base-components/formatted-monetary-amount does not match the underlying filesystem.
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.
Does this error appear only in your editor and not in the CI? If you don't have other casing on this path assets/js/base/components/formatted-monetary-amount/index.tsx
it might be a false positive. Found a similar issue. Could you maybe try to update to eslint-plugin-import@v2.8.0 and see if it still happens.
I think that's the way to go, this also removes uncertainty whether a file was converted or not. I can add a note in the knowledge file
I can see how a file could use a shared type and something specific. I also prefer local types.ts files for specific types, but this is a good topic to bring to the team. |
Looks like TotalsItem was the only on sending '-' as a value and it looks like it’s no longer passed through.
@mikejolley changes pushed |
3089564
to
4184390
Compare
PR adding types to /packages/checkout/shipping directory.
How to test the changes in this Pull Request:
Checkout branch and make sure code is free of TS warnings errors.
Testing is limited to smoke test the Cart and Checkout blocks to verify there are no regressions.
Just a note that this PR already typed FormattedMonetaryAmount component. So I just kept my file and ignored the change brought to the file here aa82c92#diff-d6ebd1dae536334d33ae51da0820ecd0896c24c57a34f5ddaebd0f8b1c98c12cL36