Display warnings and errors in the cart #3400
+282
−18
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@shopify/cli-hydrogen': patch | ||
| 'skeleton': patch | ||
| --- | ||
|
|
||
| Add cart warnings component to display feedback to users when there are issues with their cart. Includes new `InlineFeedback` component and a `CartWarnings` component for collecting and displaying cart errors and warnings in an accessible way. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import {setTestStore, test, expect, Page} from '../../fixtures'; | ||
|
|
||
| setTestStore('mockShop'); | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import {InlineFeedback} from './InlineFeedback'; | ||
| import {href, useActionData, useFetchers, type Fetcher} from 'react-router'; | ||
| import type {action as cartAction} from '~/routes/cart'; | ||
| import {useEffect, useState} from 'react'; | ||
|
|
||
| function isCartFetcher( | ||
| fetcher: Fetcher, | ||
| ): fetcher is Fetcher<CartActionData> & {key: string} { | ||
| return fetcher.formAction === href('/cart') && fetcher.formMethod === 'POST'; | ||
| } | ||
|
|
||
| type CartActionData = NonNullable< | ||
| ReturnType<typeof useActionData<typeof cartAction>> | ||
| >; | ||
| /** Returns the errors and warnings from the cart fetchers | ||
| * Normalizes the errors to provide better UX. | ||
| * | ||
| * Errors are normalized by path, warnings are normalized by code. | ||
fredericoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| export function useCartFeedback() { | ||
| const [fetcherDataMap, setFetcherDataMap] = useState< | ||
| Map<string, CartActionData> | ||
| >(new Map()); | ||
| const fetchers = useFetchers(); | ||
|
|
||
| useEffect(() => { | ||
fredericoo marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. fetcherDataMap is in the dependency array but also being updated inside this effect. In the 'submitting' case, 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 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 |
||
| let changed = false; | ||
| let newFetcherDataMap = new Map(fetcherDataMap); | ||
|
|
||
| for (const fetcher of fetchers) { | ||
| if (!isCartFetcher(fetcher)) continue; | ||
|
|
||
| switch (fetcher.state) { | ||
| case 'submitting': | ||
| newFetcherDataMap = new Map(); | ||
| changed = true; | ||
| break; | ||
| case 'loading': | ||
| if ( | ||
| fetcher.data && | ||
| fetcher.data !== newFetcherDataMap.get(fetcher.key) | ||
| ) { | ||
| changed = true; | ||
| newFetcherDataMap.set(fetcher.key, fetcher.data); | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (changed) { | ||
| setFetcherDataMap(newFetcherDataMap); | ||
| } | ||
| }, [fetchers, fetcherDataMap]); | ||
|
|
||
| const feedback: { | ||
| errors: Map<string, NonNullable<CartActionData['errors']>[number]>; | ||
| warnings: Map<string, NonNullable<CartActionData['warnings']>[number]>; | ||
| userErrors: Map<string, NonNullable<CartActionData['userErrors']>[number]>; | ||
| } = {errors: new Map(), warnings: new Map(), userErrors: new Map()}; | ||
| for (const fetcherData of fetcherDataMap.values()) { | ||
| if (fetcherData.warnings) { | ||
| for (const warning of fetcherData.warnings) { | ||
| feedback.warnings.set(warning.code, warning); | ||
| } | ||
| } | ||
| if (fetcherData.errors) { | ||
| for (const error of fetcherData.errors) { | ||
| feedback.errors.set(error.path?.join('.') ?? '_root', error); | ||
| } | ||
| } | ||
| if (fetcherData.userErrors) { | ||
| for (const userError of fetcherData.userErrors) { | ||
| feedback.userErrors.set(userError.code ?? '_root', userError); | ||
| } | ||
| } | ||
| } | ||
| return { | ||
| errors: Array.from(feedback.errors.values()), | ||
|
Contributor
There was a problem hiding this comment. The 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. |
||
| warnings: Array.from(feedback.warnings.values()), | ||
| userErrors: Array.from(feedback.userErrors.values()), | ||
| }; | ||
| } | ||
|
|
||
| /** Renders a list of warnings from the cart if there are any */ | ||
| export function CartWarnings() { | ||
fredericoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const feedback = useCartFeedback(); | ||
| if (feedback.warnings.length === 0 && feedback.userErrors.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div className="cart-warnings"> | ||
| {feedback.warnings.map((warning) => ( | ||
| <InlineFeedback | ||
| key={warning.code} | ||
| type="warning" | ||
| title={warning.message} | ||
| /> | ||
| ))} | ||
| {feedback.userErrors.map((userError) => ( | ||
| <InlineFeedback | ||
| key={userError.code} | ||
| type="error" | ||
| title={userError.message} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| interface InlineFeedbackProps { | ||
| type?: 'warning' | 'error'; | ||
| title: string; | ||
| description?: string; | ||
| } | ||
|
|
||
| /** | ||
| * An accessible inline feedback component for warnings and errors. | ||
| * Uses role="alert" to announce changes to assistive technology. | ||
|
Contributor
There was a problem hiding this comment. JSDoc doesn't match implementation The comment says:
But line 19 uses 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) |
||
| */ | ||
| export function InlineFeedback({ | ||
| type = 'warning', | ||
| title, | ||
| description, | ||
| }: InlineFeedbackProps) { | ||
| const icon = type === 'error' ? '✕' : '⚠'; | ||
|
|
||
| return ( | ||
| <div className={`inline-feedback inline-feedback--${type}`} role="status"> | ||
| <span className="inline-feedback-icon" aria-hidden="true"> | ||
| {icon} | ||
| </span> | ||
| <div className="inline-feedback-content"> | ||
| <p className="inline-feedback-title">{title}</p> | ||
| {description ? ( | ||
| <p className="inline-feedback-description">{description}</p> | ||
| ) : null} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 codebaseThis could be properly typed using the cart action type: