Improve order line discount discoverability and modal#6434
Conversation
🦋 Changeset detectedLatest commit: 83e2d05 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6434 +/- ##
==========================================
+ Coverage 47.93% 48.17% +0.23%
==========================================
Files 2579 2589 +10
Lines 45752 46043 +291
Branches 10794 10579 -215
==========================================
+ Hits 21932 22181 +249
- Misses 22571 23483 +912
+ Partials 1249 379 -870
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves discoverability of Add/Edit order line discount by moving discount editing into a unified modal UX and exposing line-discount actions from the order line row menu (and via clicking the price cell), with an accompanying “ripple” hint.
Changes:
- Introduces a new
OrderDiscountModal/OrderLineDiscountModalflow (shared base + shared form hook) and removes the legacyOrderDiscountCommonModal. - Adds an order-line row-menu action (Draft details datagrid) and makes the discounted price cell look/click behave like an entry point for editing line discounts.
- Adds a “ripple” improvement hint model and registers it globally.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ripples/allRipples.ts | Registers the new ripple hint globally. |
| src/products/components/OrderDiscountProviders/utils.ts | Updates type import to new discount modal types. |
| src/products/components/OrderDiscountProviders/types.ts | Updates type import to new discount modal types. |
| src/products/components/OrderDiscountProviders/OrderLineDiscountProvider.tsx | Updates type import to new discount modal types. |
| src/products/components/OrderDiscountProviders/OrderDiscountProvider.tsx | Updates type import to new discount modal types. |
| src/orders/ripples/orderLineDiscountDiscoverability.tsx | Adds a new ripple model describing the improvement. |
| src/orders/components/OrderSummary/OrderValue.tsx | Switches order discount editing from popover-embedded modal to the new modal component. |
| src/orders/components/OrderLineMetadataDialog/useMetadataValues.tsx | Simplifies metadata hook return typing (removes cast, returns undefined when no line). |
| src/orders/components/OrderLineMetadataDialog/OrderLineMetadataDialog.tsx | Refactors dialog header layout and adds a close icon button. |
| src/orders/components/OrderLineMetadataDialog/OrderLineMetadataDialog.test.tsx | Updates assertions to match updated header/subheader rendering. |
| src/orders/components/OrderLineMetadataDialog/OrderLineDetails/OrderLineSubheaderData.tsx | Removes the old subheader component implementation. |
| src/orders/components/OrderLineMetadataDialog/OrderLineDetails/OrderLineDetails.tsx | Replaces old “Order line: …” header/subheader with a new truncated title + metadata layout. |
| src/orders/components/OrderDraftDetailsDatagrid/messages.ts | Adds “Add discount” i18n message. |
| src/orders/components/OrderDraftDetailsDatagrid/datagrid.ts | Makes price cell readonly/no overlay editing. |
| src/orders/components/OrderDraftDetailsDatagrid/OrderDraftDetailsRowActions.tsx | Adds optional ripple indicator near the row menu button. |
| src/orders/components/OrderDraftDetailsDatagrid/OrderDraftDetailsDatagrid.tsx | Adds menu item + click-to-open line discount modal and wires modal state. |
| src/orders/components/OrderDiscountModal/useDiscountForm.ts | Adds extracted hook for discount form state/validation/conversion. |
| src/orders/components/OrderDiscountModal/useDiscountForm.test.ts | Adds unit tests for the extracted discount form hook. |
| src/orders/components/OrderDiscountModal/types.ts | Introduces the new shared discount input type. |
| src/orders/components/OrderDiscountModal/messages.ts | Adds new message descriptors for the new modal flow. |
| src/orders/components/OrderDiscountModal/OrderLineDiscountModal.tsx | Adds line-discount modal wrapper with contextual header (thumbnail/name/SKU/Qty). |
| src/orders/components/OrderDiscountModal/OrderDiscountModal.tsx | Adds order-discount modal wrapper with order-focused header/subtitle. |
| src/orders/components/OrderDiscountModal/OrderDiscountModal.module.css | Adds right-aligned numeric input styling. |
| src/orders/components/OrderDiscountModal/DiscountModalBase.tsx | Adds shared modal base layout (header + close button + actions). |
| src/orders/components/OrderDiscountModal/DiscountFormFields.tsx | Adds shared form fields for discount type/value/reason. |
| src/orders/components/OrderDiscountCommonModal/types.ts | Removes legacy common-modal types/constants. |
| src/orders/components/OrderDiscountCommonModal/index.ts | Removes legacy common-modal barrel export. |
| src/orders/components/OrderDiscountCommonModal/OrderDiscountCommonModal.tsx | Removes legacy popover/card-based discount modal implementation. |
| src/orders/components/OrderDetailsDatagrid/OrderDetailsRowActions.tsx | Adjusts row action grid columns depending on whether the second button is present. |
| src/components/Modal/DashboardModal.module.css | Adds shared modal content/truncation helper classes used by new headers. |
| src/components/Datagrid/customCells/cells.ts | Sets discounted money cell cursor to pointer. |
| src/components/Datagrid/customCells/Money/MoneyDiscountedCell.tsx | Removes inline editor and tightens draw formatting for missing values. |
| locale/defaultMessages.json | Adds/removes i18n strings corresponding to the new UI and removed components. |
| .changeset/spotty-bikes-brush.md | Adds a patch changeset describing the user-visible behavior change. |
There was a problem hiding this comment.
Pull request overview
This PR improves discount UX in order details by introducing a unified discount modal (order-level + line-level), making line discounts more discoverable via row actions/clickable price cells, and surfacing the change via a Ripple entry.
Changes:
- Replace the legacy
OrderDiscountCommonModalpopover/editor flow with newOrderDiscountModal/OrderLineDiscountModalbuilt on a sharedDiscountModalBase+useDiscountForm. - Add “Add/Edit discount” entry to order line row menu and open line-discount modal on price-cell click; update datagrid cell settings accordingly.
- Add a new Ripple announcement for the improved line discount discoverability.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ripples/allRipples.ts | Registers the new order-line discount discoverability ripple. |
| src/orders/ripples/orderLineDiscountDiscoverability.tsx | Adds the new Ripple model describing the UX improvement. |
| src/orders/components/OrderSummary/OrderValue.tsx | Switches order discount editing from popover/common modal to the new OrderDiscountModal. |
| src/orders/components/OrderDiscountModal/useDiscountForm.ts | New hook for discount form state, conversion, and validation. |
| src/orders/components/OrderDiscountModal/useDiscountForm.test.ts | Adds unit tests for useDiscountForm. |
| src/orders/components/OrderDiscountModal/types.ts | Defines OrderDiscountCommonInput type in the new modal module. |
| src/orders/components/OrderDiscountModal/messages.ts | Adds i18n messages for the new discount modal. |
| src/orders/components/OrderDiscountModal/DiscountModalBase.tsx | Shared modal shell for order and order-line discount flows. |
| src/orders/components/OrderDiscountModal/DiscountFormFields.tsx | Form UI for discount type/value/reason. |
| src/orders/components/OrderDiscountModal/OrderDiscountModal.tsx | Implements the order-level discount modal header + base composition. |
| src/orders/components/OrderDiscountModal/OrderLineDiscountModal.tsx | Implements the line-level discount modal header + base composition. |
| src/orders/components/OrderDiscountModal/*.stories.tsx | Adds Storybook coverage for both discount modals. |
| src/orders/components/OrderDiscountModal/OrderDiscountModal.module.css | Adds minor styling (right-aligned numeric input). |
| src/orders/components/OrderDraftDetailsDatagrid/messages.ts | Adds i18n message for “Add discount”. |
| src/orders/components/OrderDraftDetailsDatagrid/datagrid.ts | Makes price cell readonly/non-overlay to support click-to-edit discount. |
| src/orders/components/OrderDraftDetailsDatagrid/OrderDraftDetailsRowActions.tsx | Adds Ripple indicator around the row-menu trigger. |
| src/orders/components/OrderDraftDetailsDatagrid/OrderDraftDetailsDatagrid.tsx | Adds row-menu item + click-to-open line discount modal and renders OrderLineDiscountModal. |
| src/components/Datagrid/customCells/cells.ts | Sets pointer cursor on discounted money cells. |
| src/components/Datagrid/customCells/Money/MoneyDiscountedCell.tsx | Removes custom editor (discount editing moved to dedicated modal). |
| src/orders/components/OrderLineMetadataDialog/useMetadataValues.tsx | Tightens metadata hook return typing (no forced cast). |
| src/orders/components/OrderLineMetadataDialog/OrderLineMetadataDialog.tsx | Updates dialog header layout and close affordance. |
| src/orders/components/OrderLineMetadataDialog/OrderLineMetadataDialog.test.tsx | Updates assertions to match new header text layout. |
| src/orders/components/OrderLineMetadataDialog/OrderLineDetails/OrderLineDetails.tsx | Reworks header content + truncation and removes old subheader component usage. |
| src/orders/components/OrderLineMetadataDialog/OrderLineDetails/OrderLineSubheaderData.tsx | Removes the old subheader component. |
| src/orders/components/OrderDetailsDatagrid/OrderDetailsRowActions.tsx | Adjusts grid layout based on presence of a second action button. |
| src/components/Modal/DashboardModal.module.css | Adds .modalContent and .truncatedText utilities used by new headers. |
| src/products/components/OrderDiscountProviders/*.ts(x) | Updates imports to the new OrderDiscountModal/types location. |
| src/orders/components/OrderDiscountCommonModal/* | Removes the legacy shared discount modal implementation. |
| locale/defaultMessages.json | Updates extracted messages for new/removed UI strings. |
| .changeset/spotty-bikes-brush.md | Adds a patch changeset entry describing the UX change. |
| @@ -96,6 +105,31 @@ export const OrderDraftDetailsDatagrid = ({ | |||
| ), | |||
| onSelect: () => false, | |||
There was a problem hiding this comment.
CardMenuItem.label is used as the React key inside CardMenu and is also intended to be the visible label. Providing label: "" for multiple items leads to duplicate keys and can cause rendering issues/warnings. Set label to the translated text (e.g. product details / add/edit discount / delete) and make Icon only the icon element (since CardMenu already renders {Icon} {label}).
| const handleDiscountDataSubmission = async (errors: unknown[]) => { | ||
| if (errors.length === 0) { | ||
| await apolloClient.refetchQueries({ | ||
| include: [OrderDetailsWithMetadataDocument], | ||
| }); | ||
| } | ||
|
|
||
| closeDialog(); | ||
| notify(getDefaultNotifierSuccessErrorData(errors, intl)); | ||
| }; |
There was a problem hiding this comment.
handleDiscountDataSubmission is async and awaited refetchQueries, but it’s invoked from mutation onCompleted without awaiting/handling rejections. If refetchQueries rejects, this becomes an unhandled promise rejection. Wrap the refetch in try/catch (and still notify/close), or call void apolloClient.refetchQueries(...).catch(...) to handle failures explicitly.
| <Box | ||
| className={styles.discountSectionToggle} | ||
| display="grid" | ||
| __gridTemplateColumns="1fr auto" | ||
| alignItems="baseline" | ||
| gap={2} | ||
| cursor="pointer" | ||
| onClick={() => setDiscountsExpanded(prev => !prev)} | ||
| data-test-id="discount-section-toggle" | ||
| > |
There was a problem hiding this comment.
The discount section header is clickable via onClick, but it’s rendered as a non-interactive Box with no keyboard support/ARIA. This makes the expand/collapse control inaccessible to keyboard and assistive tech users. Use a semantic button (or add role="button", tabIndex=0, and Enter/Space key handling, plus aria-expanded/aria-controls).
| const discountAmount = line.unitDiscount.amount * line.quantity; | ||
|
|
||
| if (discountAmount === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| // Each line has at most one active discount type; use the first entry to classify it. | ||
| const type = line.discounts[0].type; | ||
| const existing = grouped.get(type); | ||
|
|
||
| if (existing) { | ||
| existing.lineCount += 1; | ||
| existing.totalAmount += discountAmount; | ||
| } else { |
There was a problem hiding this comment.
getLineDiscountsSummary classifies a line by line.discounts[0].type and attributes the full unitDiscount * quantity to that single type. The API exposes discounts as a list and doesn’t guarantee ordering, so lines with multiple discounts (e.g. manual + voucher/promotion) can be misclassified and totals can be inaccurate. Consider using the per-discount amounts from the API (OrderLineDiscount.unit/total) and summing/grouping by each discount entry’s type instead of relying on the first element and unitDiscount.
| import { formatDiscountSource } from "./formatDiscountSource"; | ||
|
|
||
| /** Jest maps `react-intl` to a mock whose formatMessage drops ICU values; use a minimal real formatter. */ | ||
| const createFormatMessageIntl = (): IntlShape => |
There was a problem hiding this comment.
Can't you just use real intl for this test? If formatter heavily relies on intl patterns but you literally replace its implementation, you test nothing
| if (isOpen !== prevIsOpen) { | ||
| setPrevIsOpen(isOpen); | ||
|
|
||
| const data = getDefaultValues(); | ||
|
|
There was a problem hiding this comment.
useDiscountForm is calling setPrevIsOpen (state update) during render when isOpen changes, which can trigger React warnings and make render behavior harder to reason about. Move the "reset when isOpen changes" logic into a useEffect that depends on isOpen (and calls reset(...) + updates previousCalculationMode).
| {undiscountedSubtotal !== orderSubtotal.gross.amount && ( | ||
| <> |
There was a problem hiding this comment.
This strict float comparison can be unstable (e.g. 0.1+0.2 rounding) and may show the tooltip even when subtotals are effectively equal. Consider comparing rounded-to-currency precision values (or using a small epsilon) before deciding to render the "Before discounts" tooltip.
| <Text | ||
| as="span" | ||
| color="default2" | ||
| className={styles.subtleLink} | ||
| onClick={() => history.push(voucherUrl(voucherId))} |
There was a problem hiding this comment.
The voucher name is rendered as a clickable <Text as="span" onClick=...>, which is not keyboard-accessible and won’t be announced as a link/button to assistive tech. Use a semantic link/button component (e.g. ButtonLink/Link) or add appropriate role, tabIndex, and keyboard handlers.
| const handleDiscountDataSubmission = async (errors: unknown[]) => { | ||
| if (errors.length === 0) { | ||
| await apolloClient.refetchQueries({ | ||
| include: [OrderDetailsWithMetadataDocument], | ||
| }); | ||
| } |
There was a problem hiding this comment.
refetchQueries is awaited without error handling. If the refetch fails/rejects, the rest of the completion handler won’t run (dialog may stay open and notifier won’t fire), and it can surface as an unhandled promise rejection since onCompleted doesn’t await this async function. Wrap the refetch in try/catch (or try/finally) so closeDialog() + notify(...) always execute.
| "saleor-dashboard": patch | ||
| --- | ||
|
|
||
| Order-level and order-line discount flows now share the same UX in Draft and Unconfirmed orders. Access Add/Edit order line discount now also from the order row menu. |
There was a problem hiding this comment.
suggestion: we could add screenshots and recording from this PR to changeset it would enchance our changelog nicely
There was a problem hiding this comment.
Do you have an example up your sleeve so I could reference @witoszekdev ?
| icon={<X size={20} />} | ||
| size="small" | ||
| variant="tertiary" | ||
| flexShrink="0" |
There was a problem hiding this comment.
nitpick: good suggestion https://github.com/saleor/saleor-dashboard/pull/6434/changes#r2978287684
Code review: use react-hook-form-aware wrappers for the discount modal form so the Macaw inputs are controlled by RHF directly instead of via ad-hoc useWatch/setValue glue. - Add HookFormRadioGroup — a generic useController wrapper for the Macaw RadioGroup that infers the field value type from the form generics, so no `as DiscountValueTypeEnum` assertion is needed on the choices or onChange handler. - Use HookFormRadioGroup for calculationMode (previously bound as `discountType`, which didn't match the actual form field). - Migrate the reason input to HookFormInput for consistency. The value input stays on <Controller> because it has a display transform (toFixed) and a cross-field error derived outside formState — left a short comment pointing at useDiscountForm.
| const orderDiscountTotal = discounts.reduce((sum, d) => sum + d.amount.amount, 0); | ||
| const lineDiscountTotal = lineDiscountsSummary.reduce((sum, e) => sum + e.totalAmount, 0); | ||
| const totalDiscountAmount = Math.round((orderDiscountTotal + lineDiscountTotal) * 100) / 100; | ||
| const ToggleIcon = discountsExpanded ? ChevronUp : ChevronDown; |
There was a problem hiding this comment.
totalDiscountAmount is rounded to 2 decimals before rendering. This can truncate amounts for currencies that use 3 fractional digits, and it’s also redundant since formatting happens at render time. Keep the full summed value and let the formatting layer handle rounding/precision (ideally using the currency’s fraction digits).
| {intl.formatNumber(undiscountedSubtotal, { | ||
| minimumFractionDigits: 2, | ||
| maximumFractionDigits: 2, | ||
| })} |
There was a problem hiding this comment.
The undiscounted subtotal tooltip forces maximumFractionDigits: 2, while the rest of the summary formatting (e.g. OrderSummaryListAmount) does not cap the maximum and can show 3+ fractional digits when needed. This can display a truncated tooltip amount for some currencies. Consider removing the hard cap or deriving the maximum fraction digits from the currency.
| <Button | ||
| data-test-id="close-button" | ||
| icon={<X size={20} />} | ||
| size="small" | ||
| variant="tertiary" | ||
| onClick={handleClose} | ||
| flexShrink="0" | ||
| /> |
There was a problem hiding this comment.
The close button is icon-only and currently has no accessible name (aria-label) or tooltip (title). Add aria-label and title (e.g. using buttonMessages.close) so screen readers can announce it and users get a native tooltip, consistent with other modals.
| <Text size={2} color="default2"> | ||
| {intl.formatMessage(discountMessages.helperText, { | ||
| source: formatDiscountSource(automaticDiscounts, intl), | ||
| })} | ||
| </Text> | ||
| <Text size={2} color="default2"> | ||
| {intl.formatMessage(discountMessages.helperSubtext)} | ||
| </Text> |
There was a problem hiding this comment.
formatDiscountSource(...) can return a React element, but it is interpolated into the message via intl.formatMessage(...), which produces a string and will stringify React nodes (e.g. "[object Object]"). Render the helper text with <FormattedMessage ... values={{ source: ... }} /> (which supports ReactNode values), or make formatDiscountSource return only plain strings.
| discount.type === OrderDiscountType.VOUCHER && voucherId ? ( | ||
| <Text | ||
| as="span" | ||
| color="default2" | ||
| className={styles.subtleLink} | ||
| onClick={() => history.push(voucherUrl(voucherId))} | ||
| title={discount.name ?? undefined} | ||
| > | ||
| {discount.name} | ||
| </Text> | ||
| ) : ( |
There was a problem hiding this comment.
The voucher name is rendered as a Text with an onClick handler, which is not keyboard-accessible and doesn't expose link semantics to assistive tech. Use a real link/button component (e.g. Link/ButtonLink) for navigation to the voucher page, and keep the visual style via CSS.
| discount.type === OrderDiscountType.VOUCHER && voucherId ? ( | ||
| <Text | ||
| as="span" | ||
| color="default2" | ||
| className={styles.subtleLink} | ||
| onClick={() => history.push(voucherUrl(voucherId))} | ||
| title={discount.name ?? undefined} | ||
| > | ||
| {discount.name} | ||
| </Text> | ||
| ) : ( |
There was a problem hiding this comment.
The voucher name navigation is implemented via Text + onClick, which is not focusable and doesn't support keyboard activation. Switch to a semantic link/button component for this navigation to meet accessibility requirements.
Screen.Recording.2026-03-24.at.00.44.07.mov