-
-
Notifications
You must be signed in to change notification settings - Fork 589
feat: open the Style panel only when explicitly requested #1775
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 all commits
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 |
|---|---|---|
|
|
@@ -173,6 +173,8 @@ function labelOverrideInvalid( | |
| interface StylePanelProps { | ||
| mapControllerRef: RefObject<MapController | null>; | ||
| onResizeStart: (event: ReactPointerEvent<HTMLDivElement>) => void; | ||
| /** Incremented when another part of the UI explicitly requests this panel. */ | ||
| openRequest?: number; | ||
| /** | ||
| * When this flips to `true` the panel collapses to its thin rail (it is not | ||
| * unmounted). Used to clear room when the notebook opens beside the map; the | ||
|
|
@@ -984,6 +986,7 @@ function RasterStyleSlider({ | |
| export function StylePanel({ | ||
| mapControllerRef, | ||
| onResizeStart, | ||
| openRequest = 0, | ||
| autoCollapse = false, | ||
| collapsed: controlledCollapsed, | ||
| onCollapsedChange, | ||
|
|
@@ -998,8 +1001,8 @@ export function StylePanel({ | |
| const updateLayer = useAppStore((s) => s.updateLayer); | ||
| const moveLayer = useAppStore((s) => s.moveLayer); | ||
| const projectName = useAppStore((s) => s.projectName); | ||
| // Style starts on its rail on every platform. Selecting a real layer below | ||
| // expands it; selecting the special Background row does not. | ||
| // Style starts on its rail on every platform and remains there until the | ||
| // user explicitly expands it. | ||
| const [internalCollapsed, setInternalCollapsed] = useState(true); | ||
| // In the shared right-sidebar mode the parent owns collapse (controlled); | ||
| // otherwise the panel manages it locally. `setIsCollapsed` routes to whichever | ||
|
|
@@ -1013,23 +1016,19 @@ export function StylePanel({ | |
| }, | ||
| [isControlled, onCollapsedChange], | ||
| ); | ||
| // Selecting a real layer expands the panel from its rail. Skipped while | ||
| // `autoCollapse` holds it closed (the notebook or a story-map presentation | ||
| // owns the workspace), so a selection made there cannot pop Style back open | ||
| // over them and defeat the auto-collapse below. | ||
| const previousSelectedLayerId = useRef(selectedLayerId); | ||
| // An explicit request (Layers → "Open Style panel") expands the panel from its | ||
| // rail. Skipped while `autoCollapse` holds it closed (the notebook or a | ||
| // story-map presentation owns the workspace), so a request made there cannot | ||
| // pop Style back open over them: the `autoCollapse` effect below acts only on | ||
| // transitions, so an expand that slipped through would stick until the | ||
| // notebook was closed and reopened. The request is still consumed so it does | ||
| // not fire later. | ||
| const previousOpenRequest = useRef(openRequest); | ||
| useEffect(() => { | ||
| const previous = previousSelectedLayerId.current; | ||
| previousSelectedLayerId.current = selectedLayerId; | ||
| if ( | ||
| !autoCollapse && | ||
| selectedLayerId && | ||
| selectedLayerId !== previous && | ||
| layers.some((candidate) => candidate.id === selectedLayerId) | ||
| ) { | ||
| setIsCollapsed(false); | ||
| } | ||
| }, [autoCollapse, layers, selectedLayerId, setIsCollapsed]); | ||
| if (openRequest === previousOpenRequest.current) return; | ||
| previousOpenRequest.current = openRequest; | ||
| if (!autoCollapse) setIsCollapsed(false); | ||
| }, [autoCollapse, openRequest, setIsCollapsed]); | ||
|
Comment on lines
+1019
to
+1031
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. The "Open Style panel" menu item in This mirrors the previous auto-expand-on-select behavior, so it may be an accepted tradeoff, but now that opening the panel is an explicit user action (vs. an incidental side effect of selecting a layer), a silent no-op is more likely to read as a bug to users. Worth considering whether the menu item should be disabled/hidden while Confidence: low-medium — plausible UX gap rather than a clear defect, since the surrounding comments suggest this drop is intentional. |
||
| // Collapse to the rail when `autoCollapse` flips on (e.g. the notebook opens), | ||
| // and restore the prior expand/collapse state when it flips back off (notebook | ||
| // closes). Both act only on the transition so the user can still toggle the | ||
|
|
||
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.
Minor icon-consistency nit:
Paletteis already used a few lines below (and in this same layer's menu) for "Open Style Manager" (line ~3757). Elsewhere in the app the Style panel itself is always represented bySlidersHorizontal(the shared-rail tab inDesktopShell.tsx, and the collapsed-rail icon inStylePanel.tsx). ReusingPalettehere makes two different actions in the same context menu look identical, and misses the icon users already associate with the Style panel.(
SlidersHorizontalwould need to be added to thelucide-reactimport list in this file.)Confidence: medium — this is a UX/consistency nit, not a functional bug.