woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
45 stars 21 forks source link

Onboarding: Remove Tax Rates Selection UI #2490

Open joemcgill opened 1 month ago

joemcgill commented 1 month ago

Part of https://github.com/woocommerce/google-listings-and-ads/issues/2458

Image

This 2nd step of onboarding includes a section for selecting whether the store uses destination based tax rates. This does not need to be chosen during initial onboarding, so we’ll remove this step and default to destination-based tax ratesduring onboarding.

Acceptance Criteria

Implementation Brief

The component that renders this section of the UI is FormContent in js/src/components/free-listings/setup-free-listings/form-content.js. Here, there is a ConditionalSection component that wraps the TaxRate component. The SetupFreeListings component that wraps this FormContent component is shared between the SavedSetupStepper for the onboarding and the EditFreeCampaign component.

Since we want to still show the ability to edit the tax rates for the EditFreeCampaign, we will need to add an optional prop like hideTaxRates to FormContent to optionally control whether those controls are shown. That prop should default to false, but if true should be used in FormContent as part of the logic used to set the value of shouldDisplayTaxRate (ref).

Test Coverage

Update the E2E tests in tests/e2e/specs/setup-mc/step-2-product-listings.test.js to remove tests that are no longer needed.

Definition Questions

  1. The handleSubmitClick callback includes a conditional check for shouldDisplayTaxRate. Is it safe to remove this conditional and the hook that this value is used to get this value since we're not longer using it?
  2. Any other adaptiveFormContext that needs to be updated?
eason9487 commented 1 month ago

1. The handleSubmitClick callback includes a conditional check for shouldDisplayTaxRate. Is it safe to remove this conditional and the hook that this value is used to get this value since we're not longer using it?

In #2458, it mentions:

This does not need to be chosen during initial onboarding, so we’ll remove this step and default to destination-based tax rates and allow this setting to be adjusted after onboarding.

Assuming this means that the tax rate setting can still be edited on the Edit Free Listings page, then the shouldDisplayTaxRate probably won't be completely removed.

2. Any other adaptiveFormContext that needs to be updated?

Not sure if I'm missing something, but this issue should not require changes to AdaptiveFormContext.

joemcgill commented 1 month ago

@eason9487

Assuming this means that the tax rate setting can still be edited on the Edit Free Listings page, then the shouldDisplayTaxRate probably won't be completely removed.

Good point. The SetupFreeListings component that wraps the FormContent component that we would be editing is shared between the SavedSetupStepper for the onboarding and the EditFreeCampaign component. Assuming we want to still show the ability to edit the tax rates for the free listings, we may need to add a new prop like hideTaxRates to optionally control whether those controls are shown. What do you think?

eason9487 commented 1 month ago

[...] Assuming we want to still show the ability to edit the tax rates for the free listings, we may need to add a new prop like hideTaxRates to optionally control whether those controls are shown. What do you think?

Looks good! Thanks!

joemcgill commented 1 week ago

@dsawardekar can you handle the updates requested here?

dsawardekar commented 1 week ago

@joemcgill I have updated the PR to address the feedback, assigning back to you for another review.

eclarke1 commented 4 days ago

Re-assigning to @dsawardekar to update the JSDocs and fix the merge conflict.