@W-19688128 Order Details supports multiship and BOPIS

[patricksullivansf]

Description

Add support to display Multiple shipments and BOPIS shipments to the My Account order details page. Prior to this change only the first shipment was shown and it was mis-leading for pick up shipments

BEFORE

AFTER DESKTOP

AFTER MOBILE

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

• (change1)

How to Test-Drive This PR

Steps to reproduce:

1. Add 1 item for store pickup then 2 separate items for ship to address (other shipment orders produce other wrong results)
2. Checkout as registered shopper (or register a guest on order confirmation page)
3. Add 2 different addresses for the 2 shipping items.
4. Proceed to place order.
5. Register a new account on Order Confirmation Page
6. View order summary

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...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

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

[cc-prodsec]

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

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Licenses	0	0	0	0	0 issues
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[aditek-sf]

LGTM

[alexvuong]

I am wondering if we can store this data somewhere to reuse? it takes up a bit of space in here

[patricksullivansf]

thanks. done.

[alexvuong]

This looks familiar 🤔 , I think I've seen this logic somewhere in multi-ship, no?

Do you think we can optimize by creating a util that will return you pick and delivery shipments based on the shipments data and it can be re-used?

[patricksullivansf]

Good memory. I see this exact logic in MultiShipOrderSummary. Consolidated.

[yhsieh1]

getStoreData(shipment.c_fromStoreId) called twice. Can we optimize it?

[patricksullivansf]

not so easily because we are inside a loop (map above) so we'd have to create more complex markup for an IIFE. IMO it's not worth the complexity given the expected length of the stores array. but I'm open to counter opinions.

[alexvuong]

you still can keep a varianble within a map fnc

{
     data.map((item, index) => {
      const storedata = getStoreData(shipment.c_fromStoreId)

     return <div> {storeData? <div>something</div> : <div>Something else</>}

    })

}

[patricksullivansf]

ok, I guess I'm outvoted. I added the local var (and the accumulated source re-pretti-ing)

[sf-henry-semaganda]

LGTM

[alexvuong]

There is a better way to handle plural for translation without conditions. See CartTitle

const CartTitle = () => {
    const {
        derivedData: {totalItems}
    } = useCurrentBasket()
    return (
        <Heading as="h1" fontSize={['xl', 'xl', 'xl', '2xl']}>
            <FormattedMessage
                defaultMessage="Cart ({itemCount, plural, =0 {0 items} one {# item} other {# items}})"
                values={{itemCount: totalItems}}
                id="cart_title.title.cart_num_of_items"
            />
        </Heading>
    )
}

[patricksullivansf]

@alexvuong thanks for the heads up. I looked up the docs and learned something new!

However, this isn't actually a plural, it two distinct headers depending on if it's a single shipment or a multiple shipment situation. Similar to a greeting if we know or don't know the name we might say "Hello, John" vs "Hello". I could use a select but I think it would just make the translator confused.

[alexvuong]

hmm if that the case. Why not break out the line instead of instructing two translations of the same terms. This will help to keep the JSX as clean as possible

<Text>
   <FormattedMessage
      defaultMessage="Pickup Address"
      id="account_order_detail.heading.pickup_address"
  />
  {pickupShipments.length > 1 ? index +1 : ''}
</Text>

[patricksullivansf]

@alexvuong I think string concatenation like that "part 1" + "part 2" is a no-no for i18n. I would assume there exists some l10n where the correct multi-shipment header is "ickuppay 2 address"

[alexvuong]

the React.Fragment will not result in a DOM node at all, so the key prop here is useless. Why not use the Box component and pass the key there?

[patricksullivansf]

@alexvuong why is the key useless in this use case? I see in the docs that key is explicitly supported; although it is admittedly unclear on how it is implemented...

[alexvuong]

The doc said if you want to use key you have to use React.Fragment explicitly. You had <> which i think may not work well with key

Fragments declared with the explicit <React.Fragment> syntax may have keys. A use case for this is mapping a collection to an array of fragments — for example, to create a description list:

[alexvuong]

See above comments on how to handle plural forms for translations

[patricksullivansf]

See ongoing discussion here

[alexvuong]

Nit: maybe better to hold this into a variable in rendering body

[alexvuong]

See above on how to handle plural form for translations

[patricksullivansf]

See ongoing discussion here