fix(client): Escape closes the card picker, and lay it out in two columns
Escape did not just fail to close the dialog — it reset the dashboard. The shared Dialog routes an escapeKeyDown close to onCancel, and this picker puts Reset to defaults in the cancel slot, so pressing Escape wiped the user's card selection. Dialog gains an optional onEscape so a dialog whose cancel slot carries another action can dismiss properly; without it the behaviour is unchanged for every other caller. The card list is now two columns. Seventeen cards in a single column made the dialog scroll, hiding most of the catalogue behind a scrollbar. Groups flow down the columns and are kept whole with break-inside, so a group is never split across the gap. Single column below the sm breakpoint. Also hardens the selection against silent loss: the persisting effect no longer writes on mount. That write was a no-op in the normal case, since the value had just been read from storage — but if the read ever fell back to the defaults (unparseable value, storage throwing), the effect immediately overwrote the real selection and made the loss permanent. Claude-Session: https://claude.ai/code/session_01AbYLvwpEaZFXktHVcqrFRd
This commit is contained in:
1 parent
a0d91abcd0
commit
e3a565d976
3 files changed
+43
-5
No files matched your search
@@ -27,6 +27,7 @@ export const DialogInput = ({
|
||||
fullWidth = false,
|
||||
additionalButtons,
|
||||
cancelSx,
|
||||
onEscape,
|
||||
}: {
|
||||
open: boolean;
|
||||
title?: string;
|
||||
@@ -43,6 +44,12 @@ export const DialogInput = ({
|
||||
additionalButtons?: ReactNode;
|
||||
/** Styles the cancel button — e.g. an auto margin to push it left. */
|
||||
cancelSx?: SxProps;
|
||||
/**
|
||||
* Called when Escape is pressed. Defaults to `onCancel`, which is right when
|
||||
* cancel simply dismisses — but not when it does something else, so a dialog
|
||||
* whose cancel slot carries another action passes its own dismiss here.
|
||||
*/
|
||||
onEscape?(): void;
|
||||
}) => {
|
||||
const { t } = useTranslation();
|
||||
const theme = useTheme();
|
||||
@@ -52,7 +59,12 @@ export const DialogInput = ({
|
||||
maxWidth={maxWidth}
|
||||
fullWidth={fullWidth}
|
||||
onClose={(_event, reason) => {
|
||||
if (reason !== "backdropClick" && onCancel) onCancel(undefined);
|
||||
if (reason === "backdropClick") return;
|
||||
if (onEscape) {
|
||||
onEscape();
|
||||
return;
|
||||
}
|
||||
if (onCancel) onCancel(undefined);
|
||||
}}
|
||||
>
|
||||
{title && (
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import Stack from "@mui/material/Stack";
|
||||
import Box from "@mui/material/Box";
|
||||
import Typography from "@mui/material/Typography";
|
||||
import FormControlLabel from "@mui/material/FormControlLabel";
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
@@ -59,19 +60,34 @@ export const CardPicker = ({
|
||||
onCancel={onReset}
|
||||
cancelText={t("pages.dashboard.picker.reset")}
|
||||
cancelSx={{ marginRight: "auto" }}
|
||||
// Escape must dismiss, not reset. The dialog sends Escape to onCancel by
|
||||
// default, and this one's cancel slot resets the selection — so without
|
||||
// this, Escape would wipe the user's cards instead of closing.
|
||||
onEscape={onClose}
|
||||
// Changes apply immediately, so Done only dismisses; it is the primary
|
||||
// action here and carries the usual contained styling.
|
||||
onConfirm={onClose}
|
||||
confirmText={t("pages.dashboard.picker.done")}
|
||||
>
|
||||
<Stack
|
||||
gap={theme.spacing(LAYOUT.MD)}
|
||||
{/*
|
||||
* Two columns: seventeen cards in one list makes the dialog scroll,
|
||||
* which hides most of the catalogue behind a scrollbar. Groups flow
|
||||
* down the columns and are kept whole, so a group is never split
|
||||
* across the gap.
|
||||
*/}
|
||||
<Box
|
||||
pt={theme.spacing(LAYOUT.SM)}
|
||||
sx={{
|
||||
columnCount: { xs: 1, sm: 2 },
|
||||
columnGap: theme.spacing(LAYOUT.XL),
|
||||
}}
|
||||
>
|
||||
{grouped.map(({ group, cards }) => (
|
||||
<Stack
|
||||
key={group}
|
||||
gap={theme.spacing(LAYOUT.XXS)}
|
||||
pb={theme.spacing(LAYOUT.MD)}
|
||||
sx={{ breakInside: "avoid" }}
|
||||
>
|
||||
<Typography
|
||||
variant="eyebrow"
|
||||
@@ -110,7 +126,7 @@ export const CardPicker = ({
|
||||
))}
|
||||
</Stack>
|
||||
))}
|
||||
</Stack>
|
||||
</Box>
|
||||
</Dialog>
|
||||
);
|
||||
};
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useMemo, useState } from "react";
|
||||
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
|
||||
|
||||
import { logger } from "@/Utils/logger";
|
||||
import { useIsAdmin } from "@/Hooks/useIsAdmin";
|
||||
@@ -50,7 +50,17 @@ export const useCardSelection = () => {
|
||||
() => readStored() ?? DEFAULT_CARD_IDS
|
||||
);
|
||||
|
||||
// Skip the write on mount. The value has just been read from storage, so
|
||||
// rewriting it achieves nothing — but if the read failed for any reason and
|
||||
// fell back to the defaults, that write would overwrite the user's real
|
||||
// selection and make the loss permanent.
|
||||
const hasMounted = useRef(false);
|
||||
|
||||
useEffect(() => {
|
||||
if (!hasMounted.current) {
|
||||
hasMounted.current = true;
|
||||
return;
|
||||
}
|
||||
try {
|
||||
window.localStorage.setItem(DASHBOARD_CARDS_STORAGE_KEY, JSON.stringify(selected));
|
||||
} catch (error) {
|
||||
|
||||
Reference in new issue
Block a user