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

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

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Sep 16, 2024

Changes proposed in this Pull Request:

Part of #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: Add Shipping Max time column in gla_shipping_times Table #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

@jorgemd24 jorgemd24 self-assigned this Sep 16, 2024
@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 16, 2024
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@                         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 Coverage Δ
js-unit-tests 63.8% <100.0%> (+0.5%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...time-setup/add-time-button/add-time-modal/index.js 100.0% <100.0%> (+88.9%) ⬆️
...me-input/edit-time-button/edit-time-modal/index.js 94.1% <100.0%> (+87.5%) ⬆️
.../shipping-time-setup/countries-time-input/index.js 91.7% <100.0%> (ø)
...g-time/shipping-time-setup/min-max-inputs/index.js 100.0% <100.0%> (ø)
...ing-time/shipping-time-setup/time-stepper/index.js 100.0% <100.0%> (ø)
js/src/utils/validateShippingTimeGroup.js 100.0% <100.0%> (ø)

Base automatically changed from add/min-max-time-inputs to update/shippings-settings-phase-1 September 16, 2024 17:07
@jorgemd24 jorgemd24 changed the title Update/add and edit shipping time inputs Update Add/Edit shipping time modals to support minimum and maximum shipping times Sep 17, 2024
@jorgemd24 jorgemd24 changed the title Update Add/Edit shipping time modals to support minimum and maximum shipping times Update Add/Edit Shipping Time Modals to support minimum and maximum shipping times Sep 17, 2024
@jorgemd24 jorgemd24 changed the title Update Add/Edit Shipping Time Modals to support minimum and maximum shipping times Update Add/Edit Shipping Time Modals to support minimum and maximum shipping times stepper Sep 17, 2024
@jorgemd24 jorgemd24 marked this pull request as ready for review September 17, 2024 17:27
@jorgemd24 jorgemd24 requested a review from a team September 17, 2024 17:35
Copy link
Contributor

@martynmjones martynmjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jorgemd24, thanks for implementing the stepper for shipping.

All is working properly although I noticed a few useability issues which we may want to address before releasing.

  1. The modal size changes when an error is displayed or hidden. If a merchant is using the + and - to set the value then the content jumps and the mouse may move off the button when the error status changes.
Modal.error.message.mov
  1. 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.
Screenshot 2024-09-19 at 11 22 05
  1. Locations are immediately grouped if the min/max values match an existing field. at the moment merchants would need to enter an identical value and then focus on a different element before they are grouped.

    As it happens when using the steppers we potentially have a situation where we want to use the + button to get to 4 but it's grouped before that can happen.

    What do you think of adding a small delay before grouping so that it would be possible to use the stepper to get to a higher number?

Screen.Recording.2024-09-19.at.15.39.25.mov

@jorgemd24
Copy link
Contributor Author

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.

Copy link
Contributor

@martynmjones martynmjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for making those adjustments @jorgemd24! Tested the updated version and both the issues I mentioned previously have been resolved.

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.

Perfect! Thank you.

@jorgemd24 jorgemd24 merged commit d23fcd9 into update/shippings-settings-phase-1 Sep 24, 2024
7 checks passed
@jorgemd24 jorgemd24 deleted the update/add-and-edit-shipping-time-inputs branch September 24, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants