Skip to content

@W-18954259 - [Multiship] Update Order Summary for multiship orders#2788

Merged
yunakim714 merged 19 commits intofeature/multishipfrom
W-18954259-multiship-order-summary
Jul 16, 2025
Merged

@W-18954259 - [Multiship] Update Order Summary for multiship orders#2788
yunakim714 merged 19 commits intofeature/multishipfrom
W-18954259-multiship-order-summary

Conversation

@yunakim714
Copy link
Collaborator

@yunakim714 yunakim714 commented Jul 11, 2025

Description

When an order has multiple shipments containing both Pickup and Delivery shipping methods, we need to show the details of all shipments on the Order Confirmation page. There can be up to 20 shipments in a single order.
Screenshot 2025-07-15 at 11 23 22 AM
Screenshot 2025-07-15 at 12 03 25 PM

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • Add a new multiship directory with the new <MultiShipConfirmation> and <MultiShipOrderSummary> components.
    • The <MultiShipConfirmation> component displays the pickup and delivery details of all shipments associated with the order
    • The <MultiShipOrderSummary> component displays which product items are Pickup and which are Delivery

How to Test-Drive This PR

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Jul 11, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@yunakim714 yunakim714 changed the base branch from develop to feature/multiship July 11, 2025 15:12
@yunakim714 yunakim714 added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Jul 11, 2025
@yunakim714 yunakim714 marked this pull request as ready for review July 15, 2025 16:08
@yunakim714 yunakim714 requested a review from a team as a code owner July 15, 2025 16:08
@patricksullivansf
Copy link
Contributor

Thank you for providing the test link!

  • I assume the "store information isn't available" is because the test link doesn't have the shipments from store ID set up correctly?
  • the UX for pick up seems not quite ideal: wouldn't the buyer need to know which items to pick up at which store?

Copy link
Contributor

@patricksullivansf patricksullivansf left a comment

Choose a reason for hiding this comment

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

awesome

import {getCreditCardIcon} from '@salesforce/retail-react-app/app/utils/cc-utils'
import useNavigation from '@salesforce/retail-react-app/app/hooks/use-navigation'

// Components
Copy link
Contributor

Choose a reason for hiding this comment

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

grouping comments are a nice touch!

}
},
{
enabled: storeIds.length > 0 && onClient
Copy link
Contributor

Choose a reason for hiding this comment

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

oh! onClient tis is a good thing to consider that i haven't paid enough attention to. But in this case why are we restricting to client API only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack I believe this was a suggestion from Cursor because this pattern is found in some other hooks calls in the retail app - I believe it prevents SSR performance issues? I don't have a strong opinion on whether we should keep this

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we want to restrict this to client only? unless I'm missing something it should be able to pre-render?

const pickupShipments = []
const deliveryShipments = []

order.shipments.forEach((shipment) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have almost the same logic in cart. might be able to consolidate into a hook.

Copy link
Contributor

@shauryemahajanSF shauryemahajanSF left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM

Copy link
Contributor

@patricksullivansf patricksullivansf left a comment

Choose a reason for hiding this comment

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

LGTM for feature branch

@yunakim714 yunakim714 merged commit 4f74527 into feature/multiship Jul 16, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants