Skip to content

Conversation

@fredericoo
Copy link
Contributor

@fredericoo fredericoo commented Jan 28, 2026

WHY are these changes introduced?

Fixes #981

WHAT is this pull request doing?

  • changes the jsx of some elements to improve accessibility and make them easier to locate with playwright
  • adds e2e tests to ensure nested cart lines work

HOW to test your changes?

run e2e tests

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@fredericoo fredericoo requested a review from a team as a code owner January 28, 2026 15:28
expect(firstProduct).toBeDefined();
expect(secondProduct).toBeDefined();

const addedLines = await request
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont love this, but it works. i am calling the proxy endpoint which then forwards the query

i would love to not have to call any gql endpoints, but i’ll still take this over

  1. modifying the jsx and clicking on the button (flaky, hard to maintain patches)
  2. opening graphiql and typing there (selecting the input, calling the query and copying the result while preserving white space was VERY challenging and boilerplatey)

<div className={className}>
<CartEmpty hidden={linesCount} layout={layout} />
<div className="cart-details">
<div aria-labelledby="cart-lines">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an element with this id did not exist

maybe we could consider adding some form of Axe a11y testing as well

this would have been caught

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I absolutely want to add that in the future!

@shopify
Copy link
Contributor

shopify bot commented Jan 28, 2026

Oxygen deployed a preview of your fb/e2e-nested-cart-lines branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment January 28, 2026 3:52 PM

Learn more about Hydrogen's GitHub integration.

</p>
<ul
aria-labelledby="cart-line-children"
className="cart-line-children"
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Hardcoded ID causes duplicate IDs when multiple parents have children

id="cart-line-children" is hardcoded. If the cart has multiple parent items with children, this creates duplicate IDs (invalid HTML) and breaks the aria-labelledby reference for screen readers.

Suggested fix:

const childrenLabelId = `cart-line-children-${id}`;
// ...
<p id={childrenLabelId} className="sr-only">
  Line items with {product.title}
</p>
<ul aria-labelledby={childrenLabelId} className="cart-line-children">

@@ -0,0 +1,131 @@
import {ProductVariant} from '@shopify/hydrogen/storefront-api-types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Imported but never used; should be removed

@@ -0,0 +1,131 @@
import {ProductVariant} from '@shopify/hydrogen/storefront-api-types';
import {setTestStore, test, expect} from '../../fixtures';
import {CART_QUERY_FRAGMENT} from '../../../templates/skeleton/app/lib/fragments';
Copy link
Contributor

Choose a reason for hiding this comment

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

Also imported but not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants