Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Onboarding: Remove Tax Rates Selection UI #2490

Closed
3 tasks
Tracked by #2458
joemcgill opened this issue Aug 5, 2024 · 7 comments · Fixed by #2542
Closed
3 tasks
Tracked by #2458

Onboarding: Remove Tax Rates Selection UI #2490

joemcgill opened this issue Aug 5, 2024 · 7 comments · Fixed by #2542

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Aug 5, 2024

Part of #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?
@eason9487
Copy link
Member

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
Copy link
Collaborator Author

@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
Copy link
Member

[...] 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
Copy link
Collaborator Author

@dsawardekar can you handle the updates requested here?

@dsawardekar
Copy link
Collaborator

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

@dsawardekar dsawardekar assigned joemcgill and unassigned dsawardekar Sep 5, 2024
@eclarke1 eclarke1 removed the Rollover label Sep 5, 2024
@joemcgill joemcgill assigned eason9487 and unassigned joemcgill Sep 5, 2024
@eclarke1 eclarke1 assigned dsawardekar and unassigned eason9487 Sep 9, 2024
@eclarke1
Copy link
Collaborator

eclarke1 commented Sep 9, 2024

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

@mikkamp
Copy link
Contributor

mikkamp commented Dec 2, 2024

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

@mikkamp mikkamp closed this as completed Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants