add default (= null) for classNames prop so it's optional for WizardTaskItem and CollapsibleBody
#9157
Conversation
- for WizardTaskItem and CollapsibleBody
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: -2 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Dang - we now get a TypeScript error if passing a // Type 'string' is not assignable to type 'null | undefined'.
<WizardTaskItem
className="gandalf"So I need to find a way to tell TypeScript that this thing can be @reykjalin @Jinksi if you know how to elegantly fix this let me know! I am trying to avoid a rabbit hole of strongly typing everything (these components are JS, i.e. inferred types - which I think is a good thing). |
|
Doing this on mobile, sorry for the lack of links and examples. You should be able to use JSDoc to provide type information that TS will use. |
I would consider migrating the 
|
Sounds good, as long as we get some benefits from strongly typing this! It feels like the tooling is forcing us to strongly type everything – once we start, we can't stop. To me, strictly typing everything seems like a heavy cost to pay – there should be a way to nudge TypeScript to correctly infer that |
|
If you wanted to keep this as JS and lean more into type inference, without explicitly declaring the prop types (nor in JSDoc), I think const WizardTaskItem = ( {
children,
title,
index,
className = '',
} ) => {
I agree, we should prefer inference wherever possible. However, writing an interface for a React component is straightforward when TS inference isn't sufficient or accurate. In this case, all the props are inferred as |
…ype to TypeScript
…:Automattic/woocommerce-payments into fix/9142-classname-optional-default-null
Great idea! I have updated the PR with this approach, it works great, with minimal code impact. I'm open to trying full-TypeScript approach, but decided against trying that myself today since I'd need to rename various files – more code impact. @Jinksi if you want to try that approach before merging, feel free to commandeer this PR 🤾🏻 |
|
Re-requested reviews, this should be good to go. I'm happy to for anyone to take this over and merge/wrap up, equally happy to handle myself when I get to it. |
There was a problem hiding this comment.
This looks good @haszari – a nice enhancement to our code quality.
I've tested this with the VAT download task list following the instructions from #8974 and confirm that the wizard renders correctly:
Also note that snapshot tests are still passing, meaning the correct classNames are being applied.
I'm open to trying full-TypeScript approach, but decided against trying that myself today since I'd need to rename various files – more code impact. @Jinksi if you want to try that approach before merging, feel free to commandeer this PR 🤾🏻
Note I've tried the TS approach in #9510 that could be merged into this PR – let me know what you think @haszari (I slightly prefer the TS approach and will merge unless you think otherwise 😎).
Test instructions followed
Note you'll need to test this with a store with account country that supports tax docs. Ideally, test with different accounts to see UI for different countries. Tip: if you set
wcpay_accounts[].deletedto a date in the past, you can onboard a new account. Or switch between accounts by setting wcpay_accounts[].deleted=NULL` on the account you want to use.
- Set up tax docs so you have a document to view at
Payments > Documents. Fake one up with this CLI command:
wp wcpay add_document_for_account [account_id] vat_invoice --mode=test --period_from="2024-04-01 00:00:00" --period_to="2024-04-30 23:59:59"- Or can use
wp wcpay backfill_merchant_vat_invoices- If you've added VAT id / tax information previously, clear it out so you can see the modal:
- Delete
company_vat_submittedcompany_vat_numbercompany_namecompany_addressfromwcpay_account_metatable.- Side note:
- Go to
Payments > Documents.- Click
Downloadfor a tax doc row.- See modal.
- Test adding valid and invalid tax info.
- Test downloading the generated tax doc – confirm the merchant details are correct.
…ve code safety and clarity (#9510)
|
Thanks @Jinksi !
I like this idea. Maybe we can add a comment to these files as we see them, i.e. intentionally not migrated, with rationale/explanation. That way the code base can be a living record of our decisions ⇒ experience of the trickier aspects of JS ⇒ TS. |
Fixes #9142
Changes proposed in this Pull Request
WizardTaskItemandCollapsibleBodycomponents to typescript, explicitly defining theclassNameas optionalclassName=nullprop in tax modal code – no longer needed.Clients currently have to pass
classNames = { null }to appease TypeScript. The prop is actually optional, only used if client wants to customise theclassof generated markup.See https://github.com/Automattic/woocommerce-payments/blob/develop/client/vat/form/tasks/vat-number-task.tsx for an example.
The goal of this PR is to streamline code like
VatNumberTaskso it doesn't need to pass meaninglessclassName={ null }.Testing instructions
WizardTaskItemandCollapsibleBody, e.g. show the VAT modal. See the following PR for instructions for testing the tax documents / tax info modal:WizardTaskItemandCollapsibleBodyand add a customclassName, to confirm that the rendered component still supports arbitrary custom classes as before.Hack on any code that uses…WizardTaskItemandCollapsibleBodyRemove theclassNames={ null }, ensure no errors/linter/typescript warnings and UI works as expected.Add custom classes viaclassNamesand ensure they are rendered correctly in generated markup.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge