From 17822b7cf65c17d6886324b9301c7bb3e20da553 Mon Sep 17 00:00:00 2001 From: gorkem-bwl Date: Thu, 3 Sep 2026 13:30:34 -0400 Subject: [PATCH] refactor(client): drop rationale comments, name the icon control width MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback from @malenacaroline on #3915: the theme pass carried too many comments, and they explain decisions rather than code. Remove 64 lines of rationale prose across 12 files. Everything taken out described *why* a value was chosen - which palette entry MUI derives what from, why a weight stays put, why a gradient lifts off `main`. That belongs in the PR description and commit history, not in an `sx` block where it goes stale the moment the palette moves. Two comments deliberately stay: - `StatusBox`: alpha() *replaces* the alpha channel rather than multiplying, so the faint grid depends on `divider` being an opaque hex. That is a live trap, not a design note. - `Table`: the `isRowSelected` JSDoc documents a real limitation of the prop (desktop table only), which a caller needs to know. Section labels (// Blues, // Hooks) are untouched. Also name the last bare number beside CONTROL_HEIGHT: the header icon links were `width: 36` next to `height: CONTROL_HEIGHT`. Now ICON_CONTROL_WIDTH. The other two `width: 36` sites are a switch track and a logo tile - same number, unrelated concepts, left alone. Gates: tsc clean, build ✓, format-check clean, lint unchanged from develop (12 errors, all pre-existing in untouched files). --- client/src/Components/design-elements/Icon.tsx | 3 --- .../src/Components/design-elements/OfflineBanner.tsx | 6 ------ client/src/Components/design-elements/PageHeader.tsx | 7 ++----- client/src/Components/design-elements/StatBox.tsx | 7 ------- client/src/Components/design-elements/Table.tsx | 4 ---- client/src/Components/design-elements/Tabs.tsx | 4 ---- client/src/Components/inputs/Button.tsx | 3 --- client/src/Components/inputs/ToggleButton.tsx | 4 ---- client/src/Components/sidebar/NavItem.tsx | 3 --- client/src/Components/sidebar/index.tsx | 3 --- client/src/Utils/Theme/Palette.ts | 12 ------------ client/src/Utils/Theme/Theme.ts | 12 ------------ client/src/Utils/Theme/constants.ts | 7 +++++++ 13 files changed, 9 insertions(+), 66 deletions(-) diff --git a/client/src/Components/design-elements/Icon.tsx b/client/src/Components/design-elements/Icon.tsx index 3832cb764..f0de97dd4 100644 --- a/client/src/Components/design-elements/Icon.tsx +++ b/client/src/Components/design-elements/Icon.tsx @@ -8,9 +8,6 @@ interface IconProps { color?: string; } -// 16 is the house size: 17 call sites ask for it explicitly against 2 for 20, -// and the sidebar clamps to 16 in CSS. A 20px default put icons above the cap -// height of the 13px text they sit beside. const Icon = ({ icon: Icon, size = 16, strokeWidth = 1.5, color }: IconProps) => { return ( { left: 0, right: 0, zIndex: theme.zIndex.snackbar, - // `error.dark` rather than `main`: in dark mode `main` is light - // enough that MUI derives near-black contrastText, which reads as - // a warning rather than an outage on the one surface where - // urgency matters. `dark` keeps a solid red bar with white text - // in both modes, and leaves `main` alone for the ~30 places that - // use it as text or a chart mark on a dark ground. backgroundColor: theme.palette.error.dark, color: "#FFFFFF", px: theme.spacing(8), diff --git a/client/src/Components/design-elements/PageHeader.tsx b/client/src/Components/design-elements/PageHeader.tsx index 28b447bc7..de544e30e 100644 --- a/client/src/Components/design-elements/PageHeader.tsx +++ b/client/src/Components/design-elements/PageHeader.tsx @@ -6,7 +6,7 @@ import { Link as RouterLink } from "react-router-dom"; import { useTranslation } from "react-i18next"; import { useTheme } from "@mui/material/styles"; import { ChevronLeft, HelpCircle, FileText, Code } from "lucide-react"; -import { CONTROL_HEIGHT } from "@/Utils/Theme/constants"; +import { CONTROL_HEIGHT, ICON_CONTROL_WIDTH } from "@/Utils/Theme/constants"; import type { ReactNode } from "react"; @@ -110,9 +110,6 @@ export const PageHeader = ({ fontSize: 14, color: theme.palette.text.secondary, lineHeight: 1.55, - // Wide enough to hold a one-line page description at this - // size; still a cap, so it wraps rather than running the - // full width of a large monitor. maxWidth: "108ch", }} > @@ -145,7 +142,7 @@ export const PageHeader = ({ display: "inline-flex", alignItems: "center", justifyContent: "center", - width: 36, + width: ICON_CONTROL_WIDTH, height: CONTROL_HEIGHT, color: theme.palette.text.secondary, borderRight: diff --git a/client/src/Components/design-elements/StatBox.tsx b/client/src/Components/design-elements/StatBox.tsx index b5e9c7834..453a83687 100644 --- a/client/src/Components/design-elements/StatBox.tsx +++ b/client/src/Components/design-elements/StatBox.tsx @@ -19,15 +19,8 @@ export const GradientBox = ({ children, palette, sx, interactive }: GradientBox) const isSmall = useMediaQuery(theme.breakpoints.down("md")); const isLight = theme.palette.mode === "light"; const paper = theme.palette.background.paper; - // In light mode `paper` is white, and lightening white is a no-op - the - // gradient collapses flat. Darken slightly towards the page ground instead. const paperStart = isLight ? darken(paper, 0.02) : lighten(paper, 0.08); const paperEnd = paper; - // Both ends stay close to `main` so MUI's derived `contrastText` still - // applies - that is white on the dark light-mode fills and near-black on - // the lighter dark-mode ones, so a bigger lift would break one or other. - // `light` is a mid-tone used for status dots and chart marks, too pale to - // use here, so the lift comes off `main` instead. const bg = palette ? `linear-gradient(135deg, ${lighten(theme.palette[palette].main, 0.12)} 0%, ${theme.palette[palette].main} 100%)` : `linear-gradient(135deg, ${paperStart} 0%, ${paperEnd} 100%)`; diff --git a/client/src/Components/design-elements/Table.tsx b/client/src/Components/design-elements/Table.tsx index 0389f4fef..84c39d1c2 100644 --- a/client/src/Components/design-elements/Table.tsx +++ b/client/src/Components/design-elements/Table.tsx @@ -220,10 +220,6 @@ export function DataTable< "& .MuiTableBody-root .MuiTableRow-root:last-child .MuiTableCell-root": { borderBottom: "none", }, - // Cells paint `background.paper`, so a background set on the row - // never shows. Selection and hover both have to be applied at - // cell level, and selection has to come first so hover layers - // on top of it rather than replacing it. "& .MuiTableBody-root .MuiTableRow-root.is-selected .MuiTableCell-root": { backgroundColor: theme.palette.action.selected, }, diff --git a/client/src/Components/design-elements/Tabs.tsx b/client/src/Components/design-elements/Tabs.tsx index 13ccaae77..ba0875c89 100644 --- a/client/src/Components/design-elements/Tabs.tsx +++ b/client/src/Components/design-elements/Tabs.tsx @@ -49,10 +49,6 @@ export const Tab = (props: CustomTabProps) => { alignItems: "flex-start", color: theme.palette.text.secondary, "&.Mui-selected": { - // Weight stays put, as on ToggleButton: bolding the active tab - // widens it, shifting every tab to its right and dragging the - // indicator to a moving target. Colour and the indicator are - // already carrying the state. color: theme.palette.primary.main, }, "&:hover": { diff --git a/client/src/Components/inputs/Button.tsx b/client/src/Components/inputs/Button.tsx index 65540b6c4..36a77efe2 100644 --- a/client/src/Components/inputs/Button.tsx +++ b/client/src/Components/inputs/Button.tsx @@ -44,9 +44,6 @@ export const ButtonInput = ({ sx, ...props }: ButtonProps) => { sx={{ textTransform: "none", height: CONTROL_HEIGHT, - // MUI's button line-height is 1.75 (22.75px at 13px), which with the - // 6px padding needs 34.75px - already over its own box at 34, and - // clipping at 32. 1.2 leaves room for tall scripts too. lineHeight: 1.2, fontWeight: 400, borderRadius: 2, diff --git a/client/src/Components/inputs/ToggleButton.tsx b/client/src/Components/inputs/ToggleButton.tsx index 0319578ae..31686d746 100644 --- a/client/src/Components/inputs/ToggleButton.tsx +++ b/client/src/Components/inputs/ToggleButton.tsx @@ -34,10 +34,6 @@ export const ToggleButtonInput = ({ sx, ...props }: ToggleButtonProps) => { }, "&.Mui-selected": { backgroundColor: selectedBg, - // Weight stays at 400 so the group sits level with Button, - // which it is routinely placed beside. The fill and the - // darker ink carry the selected state instead - a weight - // change also reflows the label width as you switch. color: theme.palette.text.primary, "&:hover": { backgroundColor: selectedHoverBg, diff --git a/client/src/Components/sidebar/NavItem.tsx b/client/src/Components/sidebar/NavItem.tsx index a07d68d7e..78f90f1f7 100644 --- a/client/src/Components/sidebar/NavItem.tsx +++ b/client/src/Components/sidebar/NavItem.tsx @@ -26,9 +26,6 @@ export const NavItem = ({ ? theme.palette.sidebar.accent : theme.palette.text.secondary; - // Both states come from the same family so selected always reads stronger - // than a hovered neighbour. MUI's default action.hover is lighter than - // action.selected on a dark ground, which inverted the two. const buttonBgColor = selected ? theme.palette.action.selected : "transparent"; const buttonBgHoverColor = selected ? theme.palette.action.selectedHover diff --git a/client/src/Components/sidebar/index.tsx b/client/src/Components/sidebar/index.tsx index e9a999ebd..c1d147519 100644 --- a/client/src/Components/sidebar/index.tsx +++ b/client/src/Components/sidebar/index.tsx @@ -73,9 +73,6 @@ export const Sidebar = () => { gap={theme.spacing(LAYOUT.SM)} borderRight={`1px solid ${theme.palette.divider}`} width={width} - // On `paper` rather than inheriting the page ground, so the nav - // reads as a surface and the content area stays the darker - // `default`. Opaque, so the blur it used to carry is gone. bgcolor={theme.palette.background.paper} sx={{ touchAction: "none", diff --git a/client/src/Utils/Theme/Palette.ts b/client/src/Utils/Theme/Palette.ts index 19ec22c85..ae3a7d89d 100644 --- a/client/src/Utils/Theme/Palette.ts +++ b/client/src/Utils/Theme/Palette.ts @@ -18,14 +18,11 @@ export const colors = { gray700: "#313131", gray900: "#151518", gray850: "#1c1c21", - // Neutral ramp, biased slightly green so it sits with the brand rather than - // against it. Opaque values - alpha-black over white reads washed out. ink900: "#101917", ink600: "#55635C", ink400: "#7C8983", surface50: "#F6F8F7", rule200: "#C7D2CC", - // Semantic colours derived from the brand rather than Material's defaults successMain: "#0D6049", errorMain: "#A02B24", warningMain: "#8C5806", @@ -60,8 +57,6 @@ export const lightPalette = { action: { rowHover: alpha(colors.gray900, HOVER.ROW), controlHover: alpha(colors.gray900, HOVER.CONTROL), - // Tinted with the brand so a selected row reads as chosen rather than - // merely dirty, and the hover step stays clearly above the resting one. selected: alpha(colors.brandGreen, 0.06), selectedHover: alpha(colors.brandGreen, 0.1), }, @@ -78,9 +73,6 @@ export const lightPalette = { disabled: colors.ink400, }, divider: colors.rule200, - // Only `main` is set. MUI derives `light`/`dark` from it, and `light` is - // consumed as a saturated colour - status dots, histogram bars, heatmap - // cells - so it must not be overridden with a pale tint. success: { main: colors.successMain, }, @@ -112,10 +104,6 @@ export const lightPalette = { export const darkPalette = { primary: { main: colors.brandGreen, - // The dark ground needs a lighter green for anything drawn as a line - // rather than a fill - focus rings especially, which were dimmer than - // their own hover state. Leaving `main` alone keeps contained buttons - // white-on-green instead of flipping them to dark text. light: colors.brandGreenLight, }, action: { diff --git a/client/src/Utils/Theme/Theme.ts b/client/src/Utils/Theme/Theme.ts index a480686a7..5e303aee0 100644 --- a/client/src/Utils/Theme/Theme.ts +++ b/client/src/Utils/Theme/Theme.ts @@ -134,9 +134,6 @@ export const theme = (mode: string, palette: any) => styleOverrides: { root: ({ theme }) => ({ "&.Mui-focusVisible": { - // Same green as the input focus ring - `main` is only - // 2.86:1 on the dark ground, so keyboard focus was - // barely visible there. outline: `1px solid ${ theme.palette.mode === "dark" ? theme.palette.primary.light @@ -196,23 +193,14 @@ export const theme = (mode: string, palette: any) => }, }, - // One border story for every outlined input - text fields, selects, - // autocompletes, date and colour pickers all route through this. - // Resting: divider. Hover: a step darker, so there is a cue without - // jumping to near-black. Focus: the brand, at 2px. MuiOutlinedInput: { styleOverrides: { - // MUI pads the inner input 8.5px top and bottom, which with an - // 18.7px line-height needs 35.7px - taller than the control - // itself, so the text was being clipped by overflow:hidden. input: { paddingTop: 0, paddingBottom: 0, height: "100%", }, root: ({ theme }) => ({ - // The Select renders a div rather than an input, so it needs - // the same treatment to sit inside the control height. "& .MuiSelect-select": { paddingTop: 0, paddingBottom: 0, diff --git a/client/src/Utils/Theme/constants.ts b/client/src/Utils/Theme/constants.ts index 1d5ed4991..3b486576f 100644 --- a/client/src/Utils/Theme/constants.ts +++ b/client/src/Utils/Theme/constants.ts @@ -25,6 +25,13 @@ export const LAYOUT = { */ export const CONTROL_HEIGHT = 32; +/** + * Width of a square-ish icon-only control that sits in a header strip - wider + * than CONTROL_HEIGHT so the icon has breathing room either side of the + * dividers between them. + */ +export const ICON_CONTROL_WIDTH = 36; + export const HOVER = { DARKEN: 0.06, // This is a coefficient for darkening function ROW: 0.025, // Overlay alpha for hoverable rows and cards