-
Notifications
You must be signed in to change notification settings - Fork 380
Display warnings and errors in the cart #3400
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
base: main
Are you sure you want to change the base?
Display warnings and errors in the cart #3400
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
3ecb1f8 to
9a80d6f
Compare
9a80d6f to
955ac0f
Compare
80251b9 to
98b056a
Compare
98b056a to
4c316a0
Compare
4c316a0 to
75e10cd
Compare
|
Please re-request my review when you want me to take a look again :) I saw you made some changes but this comment still applies so I'm assuming it's not yet ready for re-review |
66385cf to
2ead87a
Compare
09b9d0e to
cdab032
Compare
| >(new Map()); | ||
| const fetchers = useFetchers(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetcherDataMap is in the dependency array but also being updated inside this effect. In the 'submitting' case, changed is unconditionally set to true and a new empty Map is created every time. Since new Map() !== new Map() (reference inequality), React sees this as a state change even though both are empty.
The loop is prevented by the fetcher transitioning to 'loading' state quickly, not by the changed flag. This makes the code fragile — it relies on timing behaviour of React Router rather than explicit loop protection.
The 'loading' case is safer because the data comparison (fetcher.data !== newFetcherDataMap.get(fetcher.key)) naturally terminates the loop once data is stored.
Consider using the functional update pattern instead:
useEffect(() => {
setFetcherDataMap((prevMap) => {
let changed = false;
const newMap = new Map(prevMap);
for (const fetcher of fetchers) {
if (!isCartFetcher(fetcher)) continue;
switch (fetcher.state) {
case 'submitting':
if (prevMap.size > 0) {
return new Map(); // Clear on new submission
}
break;
case 'loading':
if (fetcher.data && fetcher.data !== prevMap.get(fetcher.key)) {
changed = true;
newMap.set(fetcher.key, fetcher.data);
}
break;
}
}
return changed ? newMap : prevMap;
});
}, [fetchers]); // fetcherDataMap removed from deps| } | ||
| } | ||
| return { | ||
| errors: Array.from(feedback.errors.values()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useCartFeedback hook returns errors (line 80), but the CartWarnings component only renders warnings and userErrors.
Is this intentional? If errors shouldn't display here, consider removing them from the hook's return value to avoid confusion. If they should display, we're missing that rendering.
|
|
||
| /** | ||
| * An accessible inline feedback component for warnings and errors. | ||
| * Uses role="alert" to announce changes to assistive technology. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSDoc doesn't match implementation
The comment says:
Uses role="alert" to announce changes to assistive technology.
But line 19 uses role="status".
Either update the comment to match, or consider making the role conditional:
role={type === 'error' ? 'alert' : 'status'}
Previously I had proposed using `role="status"` but I looked into this more and apparently "alert" is preferred for error messages, whereas "status" is preferred for non-error feedback (eg: cart warnings)| fetcher: FetcherWithComponents<any>, | ||
| line: CartLine, | ||
| ) { | ||
| const value = e.target.valueAsNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing NaN validation
e.target.valueAsNumber returns NaN if the input is empty or invalid. This would submit a cart update with quantity: NaN.
async function submitQuantity(
e: React.ChangeEvent<HTMLInputElement>,
fetcher: FetcherWithComponents<any>,
line: CartLine,
) {
const value = e.target.valueAsNumber;
if (Number.isNaN(value) || value < 1) {
return; // Don't submit invalid quantities
}
// ... rest
}|
|
||
| async function submitQuantity( | ||
| e: React.ChangeEvent<HTMLInputElement>, | ||
| fetcher: FetcherWithComponents<any>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly enforced right now, but I think we should never add any anys into the codebase
This could be properly typed using the cart action type:
import type {action as cartAction} from '~/routes/cart';
type CartActionResponse = Awaited<ReturnType<typeof cartAction>>;
fetcher: FetcherWithComponents<CartActionResponse>
| type="number" | ||
| defaultValue={line.quantity} | ||
| onChange={(e) => { | ||
| if (isKeyboardEvent(e)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user changes quantity via up/down arrows (triggering onChange), then clicks away (triggering onBlur), both handlers submit the same value.
The fetcher key deduplication mitigates this, but it's still unnecessary network traffic. Consider tracking whether a pending change exists:
onChange={(e) => {
if (isKeyboardEvent(e)) return; // Keyboard waits for blur
void submitQuantity(e, fetcher, line);
}}
onBlur={(e) => {
// Only submit if there's a pending keyboard change
// Current implementation submits unconditionally
void submitQuantity(e, fetcher, line);
}}

Closes #969
#3276
WHY are these changes introduced?
This PR adds the ability to display warnings to users in the cart, improving the user experience by providing clear feedback when there are issues with their cart.
WHAT is this pull request doing?
InlineWarningcomponent for displaying warning messages in an accessible wayuseCartFeedbackhook to collect and normalize errors and warnings from cart fetchersCartWarningscomponent to display warnings in the cart interfaceHOW to test your changes?
Checklist