Skip to content

Conversation

@tomalec
Copy link
Contributor

@tomalec tomalec commented Mar 22, 2022

Redo woocommerce/woocommerce-admin@8cdc828.

All Submissions:

Changes proposed in this Pull Request:

As per the Dependency Extraction Webpack Plugin installing this package in another plugin serves only type checking purposes (see WordPress/gutenberg#35630 (comment)), it would be beneficial to actually define and export those types.

I also updated the docs for the function I find the most confusing and a few return types which I find wrong.

This PR

  • Defines NumberConfig data type, export type declarations. (8cdc828)
  • Updates formatValue docs, (5d7d53f)
    • explains "based on type"
      Now it clearly explains what and in what type is returned for specific type parameter values.
    • fixes the returned value's type.
      It's not only null or string but may also be a number
  • Fix returned type of numberFormat. (baa0c5d)
    locutus/php/strings/number_format seems to always return a String.

How to test the changes in this Pull Request:

  • npm install @woocommerce/number
  • Somewhere in your codebase's JSDoc use {import('@woocommerce/number').NumberConfig}

Screenshots

image

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • [n/a] Have you written new tests for your changes, as applicable?
  • [n/a] Have you successfully run tests with your changes locally?

Changelog entry

Dev - Export data types from @woocommerce/number.

Additional notes:

Originally discussed at woocommerce/woocommerce-admin#7840 (comment)

  1. This data type could be used further in @woocommerce/currency to make the code docs shorter.
  2. Personally, I find the formatValue API confusing, as it returns type string for type='number' parameter, and type number for type='average'. I'm not sure if that was desired, given the lack of tests. Alternatively, we could change the implementation and make sure it always returns a ?string as the docs stated before. But that may be an effectively breaking change.
  3. Also We could set a default value for formatValue's type parameter, to make sure it would never return undefined, which currently may be a result of an invalid param given.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@tomalec
Copy link
Contributor Author

tomalec commented Mar 22, 2022

@psealock WDYT of merging this as a minimal set, and continue with improvements discussed in woocommerce/woocommerce-admin#7840 (comment) later?

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍 I think we can have another PR for improvements since this PR should only focus on type docs.

@tomalec
Copy link
Contributor Author

tomalec commented Mar 25, 2022

Hi @chihsuan could you merge this one then #32337 as well?

@chihsuan
Copy link
Member

Sure!

@chihsuan chihsuan merged commit 76cf618 into trunk Mar 25, 2022
@chihsuan chihsuan deleted the add/number-docs branch March 25, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: @woocommerce/number issues related to @woocommerce/number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants