Repository navigation
fix(web): bring design mode back in line with the fork's design system #241
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
Changes from 1 commit
9758b20
8f7fcbf
d530d7d
1a5800f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| // oxlint-disable t3code/no-native-title-tooltip -- Vendored design-mode island (see .fork/customizations.yaml#fork-design-mode); native titles kept to stay close to the Forge source. | ||
| import { | ||
| ChevronDownIcon, | ||
| ChevronRight, | ||
|
|
@@ -9,6 +8,7 @@ import { | |
| } from "lucide-react"; | ||
| import { memo, useCallback, useEffect, useMemo, useRef, useState, type KeyboardEvent } from "react"; | ||
|
|
||
| import { Button } from "~/components/ui/button"; | ||
| import { cn } from "~/lib/utils"; | ||
|
|
||
| import { designModeBridge } from "./designModeBridge"; | ||
|
|
@@ -339,28 +339,34 @@ function LayersRail({ runtimeTabId }: { runtimeTabId: string }) { | |
| aria-label="Filter layers" | ||
| className="h-6 w-full min-w-0 bg-transparent text-xs text-foreground outline-none placeholder:text-muted-foreground" | ||
| /> | ||
| {/* The ghost icon-xs Button, like the show-layers toggle this pairs with: the 24px | ||
| box and hover fill every icon-only control in the fork's chrome carries. */} | ||
| {query ? ( | ||
| <button | ||
| <Button | ||
| variant="ghost" | ||
| size="icon-xs" | ||
| type="button" | ||
| title="Clear filter" | ||
|
Owner
Author
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. This doesn't actually remove the native tooltip. It's also inconsistent with the control this is meant to match. The three buttons also each repeat function RailIconButton({ label, ...props }: { label: string } & ComponentProps<typeof Button>) {
return (
<Tooltip>
<TooltipTrigger render={<Button variant="ghost" size="icon-xs" aria-label={label} {...props} />} />
<TooltipPopup>{label}</TooltipPopup>
</Tooltip>
);
}Then the header is just
Owner
Author
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. Fixed in 8f7fcbf, with the helper you sketched: |
||
| aria-label="Clear filter" | ||
| onClick={() => setQuery("")} | ||
| className="flex size-5 shrink-0 items-center justify-center rounded text-muted-foreground hover:text-foreground [&_svg]:size-3" | ||
| > | ||
| <XIcon /> | ||
| </button> | ||
| </Button> | ||
| ) : ( | ||
| <button | ||
| <Button | ||
| variant="ghost" | ||
| size="icon-xs" | ||
| type="button" | ||
| title="Collapse all" | ||
| aria-label="Collapse all" | ||
| onClick={() => setExpanded(Object.fromEntries(rows.map((row) => [row.node.id, false])))} | ||
| className="flex size-5 shrink-0 items-center justify-center rounded text-muted-foreground hover:text-foreground [&_svg]:size-3.5" | ||
| > | ||
| <ChevronsDownUpIcon /> | ||
| </button> | ||
| </Button> | ||
| )} | ||
| <button | ||
| <Button | ||
| variant="ghost" | ||
| size="icon-xs" | ||
| type="button" | ||
| title="Hide layers" | ||
| aria-label="Hide layers" | ||
|
|
@@ -372,10 +378,9 @@ function LayersRail({ runtimeTabId }: { runtimeTabId: string }) { | |
| // This button goes with the rail — hand focus to the control that brings it back. | ||
| focusLayersControl(LAYERS_SHOW_BUTTON); | ||
| }} | ||
| className="flex size-5 shrink-0 items-center justify-center rounded text-muted-foreground hover:text-foreground [&_svg]:size-3.5" | ||
| > | ||
| <PanelLeftCloseIcon /> | ||
| </button> | ||
| </Button> | ||
| </header> | ||
| <div | ||
| ref={listRef} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ export function SegmentField({ | |
| type="button" | ||
| title={option.title} | ||
| aria-pressed={selected ? "true" : "false"} | ||
| data-fork-design-segment | ||
|
Owner
Author
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. With this PR, this line is no longer the full story. The selected segment declares If
Owner
Author
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. Fixed in 8f7fcbf. The selected segment's hairline is now |
||
| onClick={() => onSelect(option.value)} | ||
| className={cn( | ||
| "flex h-full flex-1 items-center justify-center rounded px-1 text-xs transition-colors [&_svg]:size-4", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,6 +203,14 @@ | |
| overflow-clip-margin: 1px; | ||
| } | ||
|
|
||
| /* The design panel's segment strips are tabs on a field track too. Their | ||
| hairline goes: the glass edge replaces it, and a light drawn inside a | ||
| 1px border would sit a pixel in from the edge. */ | ||
| &.dark[data-fork-theme="cool-darker"] [data-fork-design-segment][aria-pressed="true"] { | ||
|
Owner
Author
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. This adds a third version of "selected tab is a glass chip". The A simpler fix: build If that's too much for this PR, at least add this selector to the existing
Owner
Author
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. Addressed in 8f7fcbf, with no new rule. I didn't rebuild |
||
| border-width: 0; | ||
| box-shadow: var(--fork-glass-halo-soft), var(--fork-glass-light); | ||
| } | ||
|
|
||
| /* The composer as the same glass: the vessel is the pane, with the halo | ||
| round it and the glint, sheens and the popups' top and bottom glows on a | ||
| ::after laid over everything inside. Over, because the prompt well covers | ||
|
|
||
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.
These three buttons still show native title tooltips.
Buttonmerges its props onto the<button>it renders, sotitle="Clear filter"/"Collapse all"/"Hide layers"reach the DOM unchanged. Removing the file'soxlint-disableonly works becauset3code/no-native-title-tooltipchecks intrinsic elements (/^[a-z]/) and can't see through a component. So the PR description's line "They no longer use native title tooltips" isn't accurate.It also breaks the pairing the PR is going for. The show-layers toggle these swap with (
ForkPreviewLayersToggle.tsx) usesTooltip>TooltipTrigger render={<Button variant="ghost" size="icon-xs" …/>}>TooltipPopup. So the rail's buttons now look like it, but their tooltip is the OS one. Either wrap each in the sameTooltippattern and droptitle(aria-labelalready names them), or put the disable comment back and fix the description.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.
Fixed in 8f7fcbf. You're right:
Buttonpassedtitlethrough, so the OS tooltip was still there and the lint rule just couldn't see it. The three controls now go through a smallRailIconButtonhelper:Tooltip>TooltipTrigger render={<Button variant="ghost" size="icon-xs" aria-label={label} …/>}>TooltipPopup, the same build asForkPreviewLayersToggle. There's notitleanywhere, and the description is corrected.