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

Better implementation of Shipping Rates and Shipping Times component for MC Account Edit Settings and Setup MC Step 3 #288

Closed
ecgan opened this issue Mar 4, 2021 · 1 comment · Fixed by #1616
Labels
category: refactor The issue/PR is related to refactoring. type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@ecgan
Copy link
Member

ecgan commented Mar 4, 2021

This is related to #226 (comment) . Last night @tomalec and I had a discussion on the current shipping rates and shipping times auto save implementation in Setup MC onboarding flow and its impact on @tomalec's work on MC account edit settings page. I'm opening this issue to document the ideas in my head, to serve as a guide for @tomalec's work and for future code refactoring changes in Setup MC onboarding flow.

Current implementation

In Setup MC Step 3 page, we have a few places that calls API that saves data:

  • js/src/setup-mc/listings-stepper/setup-free-listings/form-content.js which uses useAutoSaveSettingsEffect to save automatically save settings data. This does not include shipping rate and shipping time. (This is fine and not a problem.)
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-time/shipping-time-setup/countries-time-input-form/index.js which will call upsertShippingTime in a debounced manner when users modify the shipping time in the inline edit input.
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-time/shipping-time-setup/countries-time-input/edit-time-button/edit-time-modal/index.js which will call upsert and delete shipping time when users edit or delete shipping time in the pop-up modal.
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-time/shipping-time-setup/add-time-button/add-time-modal/index.js which will call upsert shipping time when users add new shipping time.
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-rate/shipping-rate-setup/countries-price-input-form/index.js
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-rate/shipping-rate-setup/countries-price-input/edit-rate-button/edit-rate-modal/index.js
  • js/src/setup-mc/listings-stepper/setup-free-listings/shipping-rate/shipping-rate-setup/add-rate-button/add-rate-modal/index.js

For MC account edit settings page, we do not want to have auto save, instead we only want to save upon clicking save button. The last six files above present a problem and cannot be easily reused in MC account edit settings page.

Better implementation

For MC account edit settings

We duplicate the last six files above and remove the auto save logic (essentially making the component dumb). We should put the initial values for the shipping rates and shipping times into Form component (js/src/setup-mc/listings-stepper/setup-free-listings/index.js) and pass it down to the child components. When shipping rates and shipping times change, the changes should be propagated up and stored in the Form component. (Note that the above was how I implemented it in the early beginning, but along the way I changed it when I did the integration with shipping rates and shipping times API.)

Because we duplicate the files, this ensures the Setup MC onboarding flow do not break.

When users click the Save button, we can save the settings, shipping rates and shipping times in the Form's onSubmitCallback event handler.

For Setup MC Step 3 (future work)

With the above duplicated dumb components, we should then use them to replace the smart components in the Setup MC Step 3 page. The smart components should be deleted.

To implement the shipping rate and shipping time auto save feature, we can use similar technique like useAutoSaveSettingsEffect. Since the shipping rates and shipping times are stored in Form value and passed down to FormContent, we can get the values there and then come up with useAutoSaveShippingRatesEffect and useAutoSaveShippingTimesEffect.

@ecgan ecgan added type: technical debt This issue/PR represents/solves the technical debt of the project. category: refactor The issue/PR is related to refactoring. labels Mar 4, 2021
@eason9487
Copy link
Member

Components of shipping times were changed to use the shared ones in #1616, which is the last piece of this issue. Now, each group of shipping rates and shipping times components is no longer duplicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: refactor The issue/PR is related to refactoring. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
2 participants