Remove auto-save logic from EditFreeCampaign's shipping rate and tax configuration #326
Conversation
It's a modified copy of `/js/src/setup-mc/setup-stepper/setup-free-listings/index.js` and `/js/src/setup-mc/setup-stepper/setup-free-listings/form-content.js`. It has removed saving logic and `PreLaunchChecklist`. Some code doc references are added to both sides. The auto-save functionality was not complately removed, as it is still coded in leaf components. That will be removed later. (cherry picked and modified from commit 0fc7670) (cherry picked from commit 7d5a9a6)
Make it a prop for `.~/components/.../hero` and `.~/edit-free-campaign/setup-free-listings`. Hard-code it in `.~/setup-mc/setup-stepper/setup-free-listings`, to limit the number of changes there.
to remove autosaving from the leaf components, and tur them to be re-usable ones. Components are duplicated to avoid regression issues with existing onboarding flow, due to lack of tests. Cross-reference duplicates.
| onChange( | ||
| { | ||
| countries: newValue.countries, | ||
| currency: newValue.currency, | ||
| price: newValue.price, | ||
| }, | ||
| deletedCountries | ||
| ); |
There was a problem hiding this comment.
This could have been like the following?
onChange(newValue, deletedCountries)| const { | ||
| loading: loadingShippingRates, | ||
| data: shippingRates, | ||
| } = useShippingRates(); |
There was a problem hiding this comment.
As per our note in #288, I'm thinking we should not use useShippingRates in here, but instead use useShippingRates in the main big form level at js/src/edit-free-campaign/setup-free-listings/index.js, like this:
// This is the component having the main big form containing all fields in the page.
const SetupFreeListings = ( { stepHeader } ) => {
const { settings } = useSettings();
const { data: shippingRates } = useShippingRates(); // load shipping rates here, so that we can put it as initial value in the form below, just like settings.
// This will be called in manual save upon clicking save button. You will have access to the form values and the original values from the hooks above, which will allow you to compare the values and programmatically call shipping rates delete API.
const handleSubmitCallback = () => {};
// ...
return (
<Form
initialValues={ {
shipping_rate: settings.shipping_rate,
shipping_rate-rows: shippingRates, // set shipping rates as form initial values. This should then be the source of truth for the main big form, which can then be accessible for manual save and auto save. This should be an array of non-aggregated `{ countryCode, price, currency }`.
offers_free_shipping: settings.offers_free_shipping,
free_shipping_threshold: settings.free_shipping_threshold,
...
} }
validate={ handleValidate }
onSubmitCallback={ handleSubmitCallback }
>
{ ( formProps ) => {
// auto-save (including shipping rates shall be implemented inside FormContent.
return <FormContent formProps={ formProps } />;
} }
</Form>
)With the above, you can then get the shipping rates from the formProps (source of truth).
There was a problem hiding this comment.
I would consider it in future PRs. However, given the redesign #255, I start to think maybe it's better to put it deeper than the main form. We could put it inside "<SimpleShippingRate&TimeForm>" which may or may not be loaded, or lazy load, to avoid holding the main form load, if it may not be needed at all if a user has "more complex setup" an will do that manually in Google MC
| // TODO: move those handlers up to the ancestors and consider optimizing upserting. | ||
| function handleDelete( deletedCountries ) { | ||
| updateShippingRates( | ||
| shippingRates.filter( | ||
| ( rate ) => ! deletedCountries.includes( rate.countryCode ) | ||
| ) | ||
| ); | ||
| } | ||
| function handleAdd( { countries, currency, rate } ) { | ||
| // Split aggregated rate, to individial rates per country. | ||
| const addedIndividualRates = countries.map( ( countryCode ) => ( { | ||
| countryCode, | ||
| currency, | ||
| rate, // TODO: unify that | ||
| } ) ); | ||
|
|
||
| updateShippingRates( shippingRates.concat( addedIndividualRates ) ); | ||
| } | ||
| function handleChange( | ||
| { countries, currency, price }, | ||
| deletedCountries = [] | ||
| ) { | ||
| deletedCountries.forEach( ( countryCode ) => | ||
| actualCountries.delete( countryCode ) | ||
| ); | ||
|
|
||
| // Upsert rates. | ||
| countries.forEach( ( countryCode ) => { | ||
| actualCountries.set( countryCode, { | ||
| countryCode, | ||
| currency, | ||
| rate: price, // TODO: unify that | ||
| } ); | ||
| } ); | ||
| updateShippingRates( Array.from( actualCountries.values() ) ); | ||
| } |
There was a problem hiding this comment.
About your TODO comment, I'm thinking we might still need the handlers. We need to call the updateShippingRate to update this local form state, and then have a debounced callback to propagate the changes up to the main big form state. This is actually similar to CountriesPriceInputForm.
A lil bit more context on CountriesPriceInputForm: it is called a "form" because it uses local state to store user's input, however it does not use the <Form> component because we can't have nested forms.
With the debounced callback implemented, what we will see is users will be able to immediately see their changes in the input (due to the local form state), and if the price is the same as another aggregated group, after the debounce delay with the upward propagation to update the big form state, the big form will re-render and it will be merged with the aggregated group.
There was a problem hiding this comment.
Thanks, I forgot that debounce was not only to limit the flood of HTTP requests but also to limit disturbance in UI.
I restored debouncing, but keep that in CountriesPriceInput, I hope that's fine for you.
I'm not sure if/why we would have to debounce the change also from the entire form up. I think:
- In edit flow/save-on-click the big form would just record the changes passively, to submit the end result once requested by the user click.
- In onboarding/autosave flow the debouncing is done close to the input field, and then
ShippingCountriesForm.Props.onChangeis limited to: clickingAddRateModals "save",EditRateModal's "save", already delayed "input", so I think we can safely forward those events as they come.
There was a problem hiding this comment.
In edit flow/save-on-click the big form would just record the changes passively, to submit the end result once requested by the user click.
If you don't propagate the changes up to the big form, then we won't have the shipping rates data in the save-on-click form submit event handler, isn't it?
PS: the React Form component that we use there isn't like HTML form 😅 (I think that's what you are thinking).
| <CountriesPriceInputForm | ||
| initialValue={ { | ||
| countries: selectedCountryCodes, | ||
| price: '', | ||
| currency: currencyCode, | ||
| } } | ||
| /> |
There was a problem hiding this comment.
I actually thought you could have reused CountriesPriceInputForm but just remove the upsertShippingRate inside it. Thinking deeper into it, I think you might be doing the right thing here by not using it.
as suggested at #326 (comment). Update the related code docs.
To avoid distracting users while typing with countries being re-grouped. Addresses #326 (comment).
as it will not be needed once #334 will be (hopefully) merged.
tomalec
changed the title
There was a problem hiding this comment.
LGTM. Left a question that may no need to block this PR.
There is a use case that will cause a temporary inconsistent UI behavior and internal data sync, not sure if it's within the scope of this PR. Please ignore this if it will be changed in future development or not a concern. Here are the steps:
- Change any country's shipping rate
- Change the free shipping threshold
- Switch to "I have a more complex shipping setup ..."
- Switch back to "I have a fairly simple shipping setup ..."
Then the country's shipping rate will be rollbacked, but the free shipping threshold is kept.
| return ( | ||
| <div className="gla-setup-free-listings"> | ||
| <Hero stepHeader={ stepHeader } /> | ||
| { /* TODO: 'shippingTimeOption-rows' should be removed, and use shipping time API instead. */ } |
There was a problem hiding this comment.
Reference: 08bf8b1 (Just for referencing the related change as a note)
Thanks for finding that! I created a separate issue for that #338 |
|
@tomalec , I think that issue mentioned by @eason9487 happens because right now the shipping rates are stored in local state in .../configure-product-listings/shipping-rate/shipping-rate-setup/countries-form.js. When we click on the "more complex shipping rate setup", the countries-form component is unmounted and the state would then be gone. To solve this, if we propagate the changes up to the main big form (as I commented here in #326 (comment)), then it shouldn't have any problem. |
|
Cool, so we have the solution for EditFreeCampaign, now we need to find one for onboarding as well. |
Changes proposed in this Pull Request:
Create
SetupFreeListingsw/o auto-save, and use it in EditFreeCampaignIt's a modified twin of the one used in onboarding setup, but it has removed saving logic and
PreLaunchChecklist.The auto-save functionality was removed only from the "master form", but not from all its descendant components yet. 48f4b3a
Make
SetupFreeListings's step header configurable. Previously it was hard-coded, we will need that step&form to be used on different steps. 3ab3311Duplicate ShippingRate and ShippingTime for EditCampaign, to remove autosaving from the leaf components, and tur them to be re-usable ones. Components are duplicated to avoid regression issues with existing onboarding flow, due to lack of tests. c0461ba
Remove auto-save logic from the shared
ShippingRateSetup. The state is now stored locally inShippingCountriesForm, and events are propagated there so we could attach a saving strategy at this point.Update save button labels for shipping rate add/edit modals, as described at Add edit free campaign page flow #156 (comment). fa2273d
Implements a part of #156
Screenshots:
Detailed test instructions:
git co feature/156-edit-product-listings && npm install && npm run buildStill not covered here:
...and to be addressed in separate PRs to divide work, limit conflicts, cut off the work before starting redesigning "combined shipping rates and times", cut off the work before my AFK.
Additional notes:
eslint-disable jsdoc/valid-typess that were here, to make sure there will be no leftovers once both PRs are merged.