google-listings-and-ads icon indicating copy to clipboard operation
google-listings-and-ads copied to clipboard

Onboarding: Remove Tax Rates Selection UI

Open joemcgill opened this issue 1 year ago • 6 comments

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

  • [ ] The tax rate section at the bottom of step 2 is no longer present on this screen.
  • [ ] After onboarding, when you edit a free campaign, the tax rate section should show when the audience includes 'US' (retain current behavior)
  • [ ] After onboarding, when you edit a free campaign, the tax rate section should not show when the audience does not include 'US' (retain current behavior)

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?

joemcgill avatar Aug 05 '24 14:08 joemcgill

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.

eason9487 avatar Aug 06 '24 09:08 eason9487

@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?

joemcgill avatar Aug 08 '24 17:08 joemcgill

[...] 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!

eason9487 avatar Aug 09 '24 02:08 eason9487

@dsawardekar can you handle the updates requested here?

joemcgill avatar Sep 04 '24 19:09 joemcgill

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

dsawardekar avatar Sep 05 '24 08:09 dsawardekar

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

eclarke1 avatar Sep 09 '24 11:09 eclarke1

Closing this as completed since it was part of the 2.9 release.

mikkamp avatar Dec 02 '24 15:12 mikkamp