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

Add free shipping order value condition for shipping phase 1 #2686

Conversation

ianlin
Copy link
Member

@ianlin ianlin commented Nov 22, 2024

Changes proposed in this Pull Request:

Part of pcTzPl-2qP-p2, this PR implements the last design of the new shipping phase 1 for setting the free shipping order value.

The PR follows this design with slightly changes based on the discussion in pcTzPl-2qP-p2#comment-3723 and pcTzPl-2qP-p2#comment-3731.

Notable changes in this PR:

  • Create a section for free shipping Order value condition, as previously it's part of the shipping rate section.
    • New folder js/src/components/order-value-condition-section
  • Move MinumumOrderCard from js/src/components/shipping-rate-section to the new folder js/src/components/order-value-condition-section, since the card can be reused with slightly changes.
  • Change the method getCardProps() as a shared hook in js/src/components/adaptive-form/adaptive-form-context.js as there are more than one place require to use this method.
    • I'm not sure if this is the best approach, feel free to suggest anything.

Screenshots:

shipping-phase-1-order-value-condition-2.mp4

Detailed test instructions:

  1. Build the project using this PR
  2. Start the onboarding flow, stop by step 2
  3. Select My shipping settings are simple. I can manually estimate flat shipping rates. in Shipping rates section
  4. Enter an estimated shipping rate, it should be > 0
  5. See there is a new section Order value condition under Shipping rates
  6. Set the free shipping order value condition for multiple target countries
  7. Finish the onboarding flow
  8. Make sure the free shipping order value conditions are synced to the Google Merchant Center
  9. Go to Dashboard, click Edit on the free listing
  10. Redo from the step 3, and see it's working normally.

Additional details:

Changelog entry

Because we've moved minimum order card from folder

js/src/components/shipping-rate-section to
js/src/components/order-value-condition-section

Previously the mock was added in
js/src/components/shipping-rate-section/shipping-rate-section.test.js
@github-actions github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Nov 22, 2024
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.6%. Comparing base (3857f5a) to head (bed1e5a).
Report is 110 commits behind head on update/shippings-settings-phase-1.

Files with missing lines Patch % Lines
.../free-listings/setup-free-listings/form-content.js 0.0% 3 Missing and 1 partial ⚠️
.../components/adaptive-form/adaptive-form-context.js 0.0% 3 Missing ⚠️
...condition-section/order-value-condition-section.js 33.3% 2 Missing ⚠️
...ng-rate-section/flat-shipping-rates-input-cards.js 0.0% 2 Missing ⚠️
js/src/utils/getOfferFreeShippingInitialValue.js 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                         @@
##           update/shippings-settings-phase-1   #2686     +/-   ##
===================================================================
- Coverage                               63.9%   63.6%   -0.3%     
===================================================================
  Files                                    324     326      +2     
  Lines                                   5140    5191     +51     
  Branches                                1255    1261      +6     
===================================================================
+ Hits                                    3286    3302     +16     
- Misses                                  1680    1715     +35     
  Partials                                 174     174             
Flag Coverage Δ
js-unit-tests 63.6% <40.0%> (-0.3%) ⬇️

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

Files with missing lines Coverage Δ
...listings/configure-product-listings/checkErrors.js 100.0% <ø> (ø)
...inimum-order-card/calculateValueFromGroupChange.js 100.0% <ø> (ø)
...oupShippingRatesByCurrencyFreeShippingThreshold.js 100.0% <ø> (ø)
...n-section/minimum-order-card/minimum-order-card.js 96.0% <100.0%> (ø)
...-order-form-modals/add-minimum-order-form-modal.js 100.0% <ø> (ø)
...order-form-modals/edit-minimum-order-form-modal.js 100.0% <ø> (ø)
...imum-order-form-modals/minimum-order-form-modal.js 91.7% <ø> (ø)
.../minimum-order-form-modals/validateMinimumOrder.js 100.0% <ø> (ø)
...der-card/minimum-order-input-control-label-text.js 100.0% <ø> (ø)
.../minimum-order-card/minimum-order-input-control.js 92.3% <100.0%> (-0.5%) ⬇️
... and 6 more

@ianlin ianlin changed the title Add/order value condition for shipping phase 1 Add free shipping order value condition for shipping phase 1 Nov 25, 2024
@ianlin ianlin requested a review from a team November 25, 2024 05:04
@ianlin ianlin self-assigned this Nov 25, 2024
@ianlin ianlin marked this pull request as ready for review November 25, 2024 05:05
@puntope
Copy link
Contributor

puntope commented Nov 28, 2024

One issue I see is that the Save button doesn't work until you toggle "Free shipping over a specific order"

Screen.Recording.2024-11-28.at.12.22.31.mov

Previously it was implemented using radios so offer_free_shipping has an
undefined value. The validation makes sense back then.

But right now it is implemented using a checkbox, there should not be an
undefined value.
@ianlin
Copy link
Member Author

ianlin commented Nov 28, 2024

One issue I see is that the Save button doesn't work until you toggle "Free shipping over a specific order"

Thanks for catching it! Fixed in 5efbc56.

It's because previously it was implemented using radio buttons so offer_free_shipping has an undefined value. The validation makes sense back then.

But right now it is implemented using a checkbox, there should not be an undefined value, and the validation is not required now.

@@ -46,32 +48,40 @@ const MinimumOrderInputControl = ( props ) => {
} );
};

const shouldHideInput = ! values.offer_free_shipping;
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Not a big deal for me. But maybe doing if ( shouldHideInput ) { return null; } we can avoid the styling and

{ ! shouldHideInput && currency && (
						<>{ `Cost (${ currency })` }</>
					) }

chunk. I believe the currency is always available?

@puntope
Copy link
Contributor

puntope commented Nov 29, 2024

Not sure how much a blocker is this. I see that when we toggle the checkout. The value inside the minimum order value condition field disappears. This also happens in the previous implementation. So I'm approving the PR

Screen.Recording.2024-11-29.at.12.44.47.mov

@ianlin ianlin merged commit 7c9ec43 into update/shippings-settings-phase-1 Nov 29, 2024
7 checks passed
@ianlin ianlin deleted the add/order-value-condition-for-shipping-phase-1 branch November 29, 2024 12:30
@puntope puntope mentioned this pull request Dec 3, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants