-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Full @superset/ui coverage on /design + usage-drift audit page #6882
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
Open
AviPeltz
wants to merge
1
commit into
main
Choose a base branch
from
standardize-design-system
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,88 @@ | ||
| # Design system reference | ||
|
|
||
| Source of truth for `@superset/ui` conventions and known gaps. This is a living backlog, not a | ||
| finished spec — update it as decisions land (starting with the `/design/audit` review) and as new | ||
| gaps get found. | ||
|
|
||
| ## Token table | ||
|
|
||
| Defined in `apps/desktop/src/renderer/globals.css` (`:root` = dark/"ember" fallback, | ||
| `:root.light` = light fallback), mapped into Tailwind's `--color-*`/`--radius-*` namespace via | ||
| `@theme inline`. Match hardcoded colors against these before assuming a new token is needed. | ||
|
|
||
| | Tailwind class | CSS variable | | ||
| | --- | --- | | ||
| | `bg-background` / `text-background` | `--background` | | ||
| | `bg-foreground` / `text-foreground` | `--foreground` | | ||
| | `bg-card` / `text-card-foreground` | `--card`, `--card-foreground` | | ||
| | `bg-popover` / `text-popover-foreground` | `--popover`, `--popover-foreground` | | ||
| | `bg-primary` / `text-primary-foreground` | `--primary`, `--primary-foreground` | | ||
| | `bg-secondary` / `text-secondary-foreground` | `--secondary`, `--secondary-foreground` | | ||
| | `bg-muted` / `text-muted-foreground` | `--muted`, `--muted-foreground` | | ||
| | `bg-accent` / `text-accent-foreground` | `--accent`, `--accent-foreground` | | ||
| | `bg-tertiary` / `bg-tertiary-active` | `--tertiary`, `--tertiary-active` | | ||
| | `bg-destructive` / `text-destructive-foreground` | `--destructive`, `--destructive-foreground` | | ||
| | `bg-warning` / `text-warning-foreground` | `--warning`, `--warning-foreground` | | ||
| | `border-border` | `--border` | | ||
| | `border-input` | `--input` | | ||
| | `ring-ring` | `--ring` | | ||
| | `bg-chart-1` … `bg-chart-5` | `--chart-1` … `--chart-5` | | ||
| | `bg-sidebar*` / `text-sidebar*` / `border-sidebar-border` | `--sidebar*` | | ||
| | `bg-highlight` / `text-highlight-foreground` | `--highlight`, `--highlight-foreground` | | ||
| | — (used via CSS, not a utility) | `--highlight-match`, `--highlight-active` | | ||
| | `bg-fill-hover` / `bg-fill-selected` | `--fill-hover`, `--fill-selected` | | ||
| | `rounded-sm` / `rounded-md` / `rounded-lg` / `rounded-xl` | derived from `--radius` | | ||
|
|
||
| No spacing-scale tokens beyond Tailwind's defaults — spacing isn't part of this audit. | ||
|
|
||
| ## `packages/ui/src/components/ui/*` conventions | ||
|
|
||
| Observed across the 55 shadcn-derived primitives; deviations are called out below rather than | ||
| silently followed: | ||
|
|
||
| - `cn()` (from `packages/ui/src/lib/utils.ts`) merges every `className`. Only thin Radix | ||
| pass-throughs with nothing to merge (`aspect-ratio.tsx`, `collapsible.tsx`, `sonner.tsx`) skip it. | ||
| - `data-slot="<component>"` on every primitive except `sonner.tsx`, `spinner.tsx`. | ||
| - `cva()` + `VariantProps<typeof xVariants>` for anything with a real variant enum (button, badge, | ||
| alert, item, field, empty, input-group, button-group, navigation-menu, sidebar, toggle). | ||
| `toggle-group.tsx` correctly reuses `toggleVariants` via context instead of redefining it. | ||
| - `asChild` / `Slot` forwarding is reserved for components meant to render as a link/anchor | ||
| (`button.tsx`, `badge.tsx`) — not applied elsewhere, correctly. | ||
|
|
||
| ## Known deviations (documented, not yet fixed) | ||
|
|
||
| - **`input.tsx`** hand-rolls its `variant?: "default" | "ghost"` prop as a plain object instead of | ||
| `cva()`, and isn't typed with `VariantProps<>` like `button.tsx`/`badge.tsx` are. Bring it in line | ||
| with the rest of the variant-bearing primitives. | ||
| - **Showcase gaps**: `chart.tsx`, `form.tsx`, and the full `sidebar.tsx` nav-shell aren't demoed | ||
| anywhere on `apps/web/src/app/design` (primitives page). `sidebar-card.tsx` *is* shown, via | ||
| `DataSection.tsx` — only the nav shell itself is missing. | ||
|
|
||
| ## Promotion-candidate backlog | ||
|
|
||
| Tracked today on `/design/superset` → "Shared app components" → Desktop renderer, with import-site | ||
| counts. Coupling assessed by reading each implementation: | ||
|
|
||
| | Component | Path | Coupling | Verdict | | ||
| | --- | --- | --- | --- | | ||
| | `PickerTrigger` | `apps/desktop/src/renderer/components/PickerTrigger` | Pure UI — wraps `@superset/ui/button` with `cn()`, no app-specific imports | Ready to promote as-is | | ||
| | `AgentSelect` | `.../renderer/components/AgentSelect` | Wraps `@superset/ui/select` cleanly, but hardcodes `useNavigate` (`@tanstack/react-router`) and desktop's preset-icon asset store | Needs navigation/icon-lookup lifted to props before promotion | | ||
| | `MarkdownRenderer` | `.../renderer/components/MarkdownRenderer` | Uses `cn()`, but pulls `useMarkdownStyle` from a renderer Zustand store plus a `SelectionContextMenu` subcomponent | Style-config logic is portable; the global-store hook needs to become a prop | | ||
| | `ColorSelector` | `.../renderer/components/ColorSelector` | Uses `cn()` and tokens correctly, but imports `PROJECT_COLORS`/`PROJECT_COLOR_DEFAULT` from an app-domain constants file | Needs the palette passed as a prop to become UI-only | | ||
|
|
||
| Other candidates on the same list (`HotkeyMenuShortcut`, `ColorSelector`, `HotkeyTooltip`, | ||
| `EmojiTextInput`, `ThemeSwatch`, `UpdatesPill`, `OpenInButton`, `AgentModelSelect`, | ||
| `MarkdownEditor`) haven't been individually assessed yet — do that before promoting them. | ||
|
Comment on lines
+73
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove Line 71 already records a coupling assessment and promotion requirement for 🤖 Prompt for AI Agents |
||
|
|
||
| ## Usage-drift backlog (found, not yet fixed) | ||
|
|
||
| - **Hardcoded color literals**: 40 files in `apps/desktop/src` use a hex/rgb literal instead of a | ||
| token, outside theme/story/test files. | ||
| - **Inline styles**: 86 files use `style={{...}}`; many are legitimately dynamic (runtime-computed | ||
| values, CSS properties with no Tailwind equivalent) — each needs a static/dynamic judgment call, | ||
| not a blind rewrite. | ||
| - **Standard-component usage variance**: see `/design/audit` — Button usage alone spans ~11 distinct | ||
| visual treatments across canonical-variant-with-override and fully bespoke non-`Button` elements. | ||
| Badge and Input show smaller-scale versions of the same pattern. This is the highest-leverage | ||
| fix, and it's blocked on a human decision (which treatments are canonical) before any migration | ||
| or lint rule can be written — see `/design/audit` and reply with verdicts per bucket. | ||
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
35 changes: 35 additions & 0 deletions
35
apps/web/src/app/design/audit/components/BadgeAuditSection/BadgeAuditSection.tsx
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,35 @@ | ||
| import { Badge } from "@superset/ui/badge"; | ||
|
|
||
| import { ComponentCard } from "../../../components/ComponentCard"; | ||
| import { ShowcaseSection } from "../../../components/ShowcaseSection"; | ||
|
|
||
| export function BadgeAuditSection() { | ||
| return ( | ||
| <ShowcaseSection | ||
| id="badge-audit" | ||
| index="02" | ||
| title="Badge — same disease, smaller scale" | ||
| description="31 files, 49 occurrences. Mostly clean; one repeated override." | ||
| > | ||
| <ComponentCard | ||
| title="1 · Canonical, correct" | ||
| importPath="PermissionsSettings.tsx" | ||
| copyable={false} | ||
| description="variant only, no className." | ||
| > | ||
| <Badge variant="secondary">Admin</Badge> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="2 · Unofficial "compact" size" | ||
| importPath="OrganizationSettings.tsx, PresetRow.tsx (copy-pasted ~5x)" | ||
| copyable={false} | ||
| description="className="text-[10px] h-4 px-1.5" — badgeVariants has no size axis, so every caller re-derives one." | ||
| > | ||
| <Badge variant="secondary" className="h-4 px-1.5 text-[10px]"> | ||
| You | ||
| </Badge> | ||
| </ComponentCard> | ||
| </ShowcaseSection> | ||
| ); | ||
| } |
1 change: 1 addition & 0 deletions
1
apps/web/src/app/design/audit/components/BadgeAuditSection/index.ts
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 @@ | ||
| export { BadgeAuditSection } from "./BadgeAuditSection"; |
208 changes: 208 additions & 0 deletions
208
apps/web/src/app/design/audit/components/ButtonAuditSection/ButtonAuditSection.tsx
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,208 @@ | ||
| "use client"; | ||
|
|
||
| import { Button } from "@superset/ui/button"; | ||
| import { | ||
| ArrowDown, | ||
| ChevronDown, | ||
| ChevronRight, | ||
| Copy, | ||
| ExternalLink, | ||
| MoreVertical, | ||
| Plus, | ||
| RefreshCw, | ||
| Terminal, | ||
| TriangleAlert, | ||
| X, | ||
| } from "lucide-react"; | ||
|
|
||
| import { ComponentCard } from "../../../components/ComponentCard"; | ||
| import { ShowcaseSection } from "../../../components/ShowcaseSection"; | ||
|
|
||
| export function ButtonAuditSection() { | ||
| return ( | ||
| <ShowcaseSection | ||
| id="button-audit" | ||
| index="01" | ||
| title="Button — treatments in the wild" | ||
| description="213 files, 449 real <Button> call sites, plus 363 raw <button> occurrences. Sampled below." | ||
| > | ||
| <ComponentCard | ||
| title="1 · Canonical, correct" | ||
| importPath="WorkspacesListView.tsx" | ||
| copyable={false} | ||
| description="variant + size only, no className. This is the target." | ||
| > | ||
| <Button variant="ghost" size="icon" aria-label="More options"> | ||
| <MoreVertical /> | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="2 · Canonical + layout-only className" | ||
| importPath="CommitInput.tsx" | ||
| copyable={false} | ||
| description="flex-1/gap are fine. Real file also adds h-7 text-xs, redundant with size="sm" — trim on migration." | ||
| > | ||
| <Button variant="secondary" size="sm" className="flex-1 gap-1.5"> | ||
| Commit | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="3 · Reimplements existing size="xs"" | ||
| importPath="DeleteWorkspaceDialog.tsx (+6 more, copy-pasted verbatim)" | ||
| copyable={false} | ||
| description="size="sm" className="h-7 px-3 text-xs" — size="xs" already does this correctly." | ||
| > | ||
| <Button variant="ghost" size="sm" className="h-7 px-3 text-xs"> | ||
| Cancel | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="4 · Undersized icon override" | ||
| importPath="ChangesHeader.tsx, useOrderedSections.tsx" | ||
| copyable={false} | ||
| description="size="icon" className="size-6 p-0" — smaller than any defined icon size (icon-xs is size-7)." | ||
| > | ||
| <Button variant="ghost" size="icon" className="size-6 p-0"> | ||
| <RefreshCw className="size-3.5" /> | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="5a · Unofficial "warning" color" | ||
| importPath="PaymentFailedBanner.tsx" | ||
| copyable={false} | ||
| description="No warning variant exists in buttonVariants — this is hand-mixed from tokens." | ||
| > | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className="ml-auto h-7 shrink-0 border-warning/40 bg-warning/10 px-2.5 text-xs text-warning hover:bg-warning/20" | ||
| > | ||
| <TriangleAlert className="size-3.5" /> | ||
| Update payment | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="5b · Unofficial "destructive-ghost" color" | ||
| importPath="FileDiffHeader.tsx" | ||
| copyable={false} | ||
| description="Same problem, different recolor — a second hand-mixed variant that doesn't exist." | ||
| > | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-6 text-destructive hover:text-destructive hover:bg-destructive/10" | ||
| > | ||
| <X className="size-3.5" /> | ||
| </Button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="6 · Hand-built segmented group" | ||
| importPath="AddTabButton.tsx" | ||
| copyable={false} | ||
| description="Fakes ButtonGroup with manual rounded-*-none + shared borders instead of composing it." | ||
| span | ||
| > | ||
| <div className="flex"> | ||
| <Button | ||
| variant="ghost" | ||
| className="h-7 gap-1 rounded-r-none border border-border/60 bg-muted/30 pl-2 pr-1.5 text-xs text-muted-foreground hover:bg-accent/60 hover:text-foreground" | ||
| > | ||
| <Terminal className="size-3.5" /> | ||
| Terminal | ||
| </Button> | ||
| <Button | ||
| variant="ghost" | ||
| className="h-7 gap-1 rounded-none border border-l-0 border-border/60 bg-muted/30 px-1.5 text-xs text-muted-foreground hover:bg-accent/60 hover:text-foreground" | ||
| > | ||
| Browser | ||
| </Button> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-7 rounded-l-none border border-l-0 border-border/60 bg-muted/30 px-1 text-muted-foreground hover:bg-accent/60 hover:text-foreground" | ||
| > | ||
| <ChevronDown className="size-3" /> | ||
| </Button> | ||
| </div> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="7 · Bespoke flat-fill icon button" | ||
| importPath="NewWorkspaceButton.tsx" | ||
| copyable={false} | ||
| description="Raw <button>, never touches @superset/ui/button. Flat bg-fill-hover family." | ||
| > | ||
| <button | ||
| type="button" | ||
| className="group flex size-8 items-center justify-center rounded-md bg-fill-hover transition-colors hover:bg-fill-selected" | ||
| > | ||
| <Plus className="size-4" /> | ||
| </button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="8 · Bespoke bordered-circle FAB" | ||
| importPath="ScrollToBottomButton.tsx" | ||
| copyable={false} | ||
| description="Raw <button>. Different family again: full radius, visible border, bg-background." | ||
| > | ||
| <button | ||
| type="button" | ||
| className="flex size-8 items-center justify-center rounded-full border border-border bg-background text-muted-foreground transition-colors hover:bg-muted hover:text-foreground" | ||
| > | ||
| <ArrowDown className="size-4" /> | ||
| </button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="9 · Bespoke glass/backdrop-blur toolbar button" | ||
| importPath="CodeBlockView.tsx (independently re-implemented in error.tsx)" | ||
| copyable={false} | ||
| description="Raw <button>. A third distinct family, duplicated by two different authors instead of shared." | ||
| > | ||
| <button | ||
| type="button" | ||
| className="flex h-6 w-6 items-center justify-center rounded border border-border bg-background/80 backdrop-blur transition-colors hover:bg-accent" | ||
| > | ||
| <Copy className="size-3" /> | ||
| </button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="10 · Bespoke borderless hover-icon button" | ||
| importPath="ProjectHeader.tsx" | ||
| copyable={false} | ||
| description="Raw <button>. No default bg/border at all — a fourth family." | ||
| > | ||
| <button | ||
| type="button" | ||
| className="shrink-0 rounded p-1 transition-colors hover:bg-fill-hover" | ||
| > | ||
| <ChevronRight className="size-4 text-muted-foreground" /> | ||
| </button> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="11 · Bare, no-chrome clickable icon" | ||
| importPath="MergedPortBadge.tsx, error.tsx, WorkspaceListItem.tsx (div role="button")" | ||
| copyable={false} | ||
| description="No radius/padding/bg at all — opacity-0 until group-hover. Hover this card to see it." | ||
| > | ||
| <div className="group flex size-8 items-center justify-center"> | ||
| <button | ||
| type="button" | ||
| className="text-muted-foreground opacity-0 transition-opacity hover:text-primary focus-visible:opacity-100 focus-visible:outline-none group-hover:opacity-100" | ||
| > | ||
| <ExternalLink className="size-3.5" /> | ||
| </button> | ||
| </div> | ||
| </ComponentCard> | ||
| </ShowcaseSection> | ||
| ); | ||
| } |
1 change: 1 addition & 0 deletions
1
apps/web/src/app/design/audit/components/ButtonAuditSection/index.ts
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 @@ | ||
| export { ButtonAuditSection } from "./ButtonAuditSection"; |
36 changes: 36 additions & 0 deletions
36
apps/web/src/app/design/audit/components/InputAuditSection/InputAuditSection.tsx
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,36 @@ | ||
| import { Input } from "@superset/ui/input"; | ||
|
|
||
| import { ComponentCard } from "../../../components/ComponentCard"; | ||
| import { ShowcaseSection } from "../../../components/ShowcaseSection"; | ||
|
|
||
| export function InputAuditSection() { | ||
| return ( | ||
| <ShowcaseSection | ||
| id="input-audit" | ||
| index="03" | ||
| title="Input — one recurring override" | ||
| description="54 files, 84 occurrences. Mostly clean; one repeated full-chrome strip." | ||
| > | ||
| <ComponentCard | ||
| title="1 · Canonical, correct" | ||
| importPath="RenameBranchDialog.tsx" | ||
| copyable={false} | ||
| description="Default variant, no className." | ||
| > | ||
| <Input placeholder="Branch name" className="max-w-56" /> | ||
| </ComponentCard> | ||
|
|
||
| <ComponentCard | ||
| title="2 · Borderless inline override" | ||
| importPath="PromptGroup.tsx (repeated with slight variation twice)" | ||
| copyable={false} | ||
| description="Strips the component's chrome entirely via className. Input already has variant="ghost" — close, but doesn't match (padding/height differ) — worth comparing directly." | ||
| > | ||
| <Input | ||
| placeholder="Workspace name (optional)" | ||
| className="h-auto min-w-0 flex-1 border-none bg-transparent px-0 text-base font-medium shadow-none placeholder:text-muted-foreground/40 focus-visible:ring-0" | ||
| /> | ||
| </ComponentCard> | ||
| </ShowcaseSection> | ||
| ); | ||
| } |
1 change: 1 addition & 0 deletions
1
apps/web/src/app/design/audit/components/InputAuditSection/index.ts
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 @@ | ||
| export { InputAuditSection } from "./InputAuditSection"; |
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the sidebar showcase-gap statement.
apps/web/src/app/design/audit/components/SidebarAuditSection/SidebarAuditSection.tsxlines 36-67 already rendersSidebarProvider,Sidebar, header, content, menu, and footer primitives. The claim that the sidebar nav shell is not demoed anywhere underapps/web/src/app/designis false.Remove the sidebar entry, or limit the statement to the
/designprimitives route. This reference can otherwise cause duplicate showcase work.🤖 Prompt for AI Agents