search

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
47 stars 21 forks source link

Update Add/Edit Shipping Time Modals to support minimum and maximum shipping times stepper #2609

Closed jorgemd24 closed 2 months ago

jorgemd24 commented 2 months ago

Changes proposed in this Pull Request:

Part of https://github.com/woocommerce/google-listings-and-ads/pull/2594

Since we're now working with min and max shipping times, the "Edit" and "Add" modals need to be updated to use the stepper component when creating or editing shipping times. This PR introduces a new component, MinMaxShippingTimes, which includes steppers for both the minimum and maximum shipping times.

image

image

The errors in the modals are displayed within the form instead of beneath the stepper component to prevent issues like this:

image

Screenshots:

Detailed test instructions:

  1. Follow the steps mentioned in this PR to update the database: https://github.com/woocommerce/google-listings-and-ads/pull/2520
  2. Navigate to Dashboard -> Free Listings -> Edit
  3. Add and edit shipping times using the modals, and verify that the min and max times are updated correctly.

Additional details:

In this commit 8652d24 I updated the onBlur function to align with onIncrement. However, I'm unsure what's best—whether we should allow users to enter any value and display errors in the form, or restrict input to values within the allowed range, like preventing negative numbers. Any advice would be appreciated.

Changelog entry

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.8%. Comparing base (6a31818) to head (c5e4dc3). Report is 22 commits behind head on update/shippings-settings-phase-1.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## update/shippings-settings-phase-1 #2609 +/- ## =================================================================== + Coverage 63.3% 63.8% +0.5% =================================================================== Files 322 323 +1 Lines 5119 5134 +15 Branches 1251 1254 +3 =================================================================== + Hits 3242 3278 +36 + Misses 1704 1683 -21 Partials 173 173 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `63.8% <100.0%> (+0.5%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [...time-setup/add-time-button/add-time-modal/index.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Fcomponents%2Ffree-listings%2Fconfigure-product-listings%2Fshipping-time%2Fshipping-time-setup%2Fadd-time-button%2Fadd-time-modal%2Findex.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2NvbXBvbmVudHMvZnJlZS1saXN0aW5ncy9jb25maWd1cmUtcHJvZHVjdC1saXN0aW5ncy9zaGlwcGluZy10aW1lL3NoaXBwaW5nLXRpbWUtc2V0dXAvYWRkLXRpbWUtYnV0dG9uL2FkZC10aW1lLW1vZGFsL2luZGV4Lmpz) | `100.0% <100.0%> (+88.9%)` | :arrow_up: | | [...me-input/edit-time-button/edit-time-modal/index.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Fcomponents%2Ffree-listings%2Fconfigure-product-listings%2Fshipping-time%2Fshipping-time-setup%2Fcountries-time-input%2Fedit-time-button%2Fedit-time-modal%2Findex.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2NvbXBvbmVudHMvZnJlZS1saXN0aW5ncy9jb25maWd1cmUtcHJvZHVjdC1saXN0aW5ncy9zaGlwcGluZy10aW1lL3NoaXBwaW5nLXRpbWUtc2V0dXAvY291bnRyaWVzLXRpbWUtaW5wdXQvZWRpdC10aW1lLWJ1dHRvbi9lZGl0LXRpbWUtbW9kYWwvaW5kZXguanM=) | `94.1% <100.0%> (+87.5%)` | :arrow_up: | | [.../shipping-time-setup/countries-time-input/index.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Fcomponents%2Ffree-listings%2Fconfigure-product-listings%2Fshipping-time%2Fshipping-time-setup%2Fcountries-time-input%2Findex.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2NvbXBvbmVudHMvZnJlZS1saXN0aW5ncy9jb25maWd1cmUtcHJvZHVjdC1saXN0aW5ncy9zaGlwcGluZy10aW1lL3NoaXBwaW5nLXRpbWUtc2V0dXAvY291bnRyaWVzLXRpbWUtaW5wdXQvaW5kZXguanM=) | `91.7% <100.0%> (ø)` | | | [...g-time/shipping-time-setup/min-max-inputs/index.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Fcomponents%2Ffree-listings%2Fconfigure-product-listings%2Fshipping-time%2Fshipping-time-setup%2Fmin-max-inputs%2Findex.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2NvbXBvbmVudHMvZnJlZS1saXN0aW5ncy9jb25maWd1cmUtcHJvZHVjdC1saXN0aW5ncy9zaGlwcGluZy10aW1lL3NoaXBwaW5nLXRpbWUtc2V0dXAvbWluLW1heC1pbnB1dHMvaW5kZXguanM=) | `100.0% <100.0%> (ø)` | | | [...ing-time/shipping-time-setup/time-stepper/index.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Fcomponents%2Ffree-listings%2Fconfigure-product-listings%2Fshipping-time%2Fshipping-time-setup%2Ftime-stepper%2Findex.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2NvbXBvbmVudHMvZnJlZS1saXN0aW5ncy9jb25maWd1cmUtcHJvZHVjdC1saXN0aW5ncy9zaGlwcGluZy10aW1lL3NoaXBwaW5nLXRpbWUtc2V0dXAvdGltZS1zdGVwcGVyL2luZGV4Lmpz) | `100.0% <100.0%> (ø)` | | | [js/src/utils/validateShippingTimeGroup.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2609?src=pr&el=tree&filepath=js%2Fsrc%2Futils%2FvalidateShippingTimeGroup.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL3V0aWxzL3ZhbGlkYXRlU2hpcHBpbmdUaW1lR3JvdXAuanM=) | `100.0% <100.0%> (ø)` | |
jorgemd24 commented 2 months ago

Thanks @martynmjones for your helpful comments! I’ve addressed points 1 and 3.

Regards the second one:

Invalid fields aren't highlighted when attempting to save changes. If a merchant is shipping to a number of countries and has different settings for each then it would be quite difficult to spot where the issue is.

I agree that it's not easy to see which one is incorrect, but this behaviour hasn't changed compared to the one in the develop branch.

image

I believe the issue is that we're treating shipping_times as a group of inputs rather than individual inputs, which makes it harder to display errors for each input separately. I'll create an issue for this since I think it goes beyond the scope of this PR.