-
Notifications
You must be signed in to change notification settings - Fork 90
Migrate to new layout system #8570
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
We won't use it for now so we should come back and add it in when it's not dead code
ab75504
to
ce56ec9
Compare
d9bf6a4
to
127e749
Compare
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.
A little bit of self-review in narrative form to help the brave souls who review for me. Thanks and apologies for the size of this PR!
type: LayoutType.Simple | ||
areaType: AreaType | ||
} | ||
export type Layout = SimpleLayout | SplitLayout | PaneLayout |
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.
This is where to start. Layout
is a recursive structure of split layouts, pane layouts (which are an extension around splits), and simple layouts or "areas".
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.
Recursively parsing a layout from a JSON string can fail for a number of reasons. If it fails in a way that we can "heal", we do that, but if not we return an error so that higher levels can decide what to do. This could be made more sophisticated in the future (or now if we think it's needed), but for example a Split layout will drop any children that return errors while parsing.
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.
The parsing utils got their own file because I felt they were specialized enough, but this utils contains helper functions for loading layouts from storage, for manipulating a layout, and even for creating Tailwind CSS classes based on layout data, so it just got called "utils".
Please let me know if you'd like to see any unit tests for functions in this file. If you don't see it as critical, I will make an issue to add more when I find time.
togglePane: () => {}, | ||
/** Kind of a feature flag, remove in future */ | ||
enableContextMenus: false, | ||
}) |
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.
The <LayoutRootNode>
component wraps the recursive <LayoutNode>
component in a context that includes all the mutation methods for the current layout (which is not stored here, but rather in appActor
, to be discussed below), and the area and action libraries that connect the areaType
and actionType
properties within a layout to actual implementations.
This allows us to decouple the layout configuration from the implementations, which you can see demonstrated in <TestLayout>
, which provides a testAreaLibrary instead of the defaultAreaLibrary, and provides no actionLibrary
at all, leaning back on the nullActionLibrary
defined above.
> | ||
{layout.children.map((a, i, arr) => { | ||
const disableResize = !shouldEnableResizeHandle(a, i, arr) | ||
const disableFlex = shouldDisableFlex(a, layout) |
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.
This is a problematic trick I've used to opt out of the split area sizing while a Pane layout within a Split layout is collapsed (having no activeIndices
). While it does the trick nicely of locking the size to the toolbar (since react-resizable-panels
uses flexbox for its sizing), it creates a slightly imprecise resizing behavior on the resize handles that are still present, and definitely feels like a hack.
This is one of my misgivings with how I've done this: because a Pane layout can expand and collapse, it needs to propagate its resizing "upwards" to its parent Split layout in our conventional "sidebar"-style arrangement. This leads to oddly specific mutations for toggling, and oddly specific layout logic like disableFlex
here. But I do find the nesting structure, and the types within it, very powerful and intuitive besides that. I'm interested if anyone has thoughts on this.
engineCommandManager: engineCommandManager, | ||
sceneInfra: sceneInfra, | ||
sceneEntitiesManager: sceneEntitiesManager, | ||
layout: defaultLayout, |
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.
The layout is no longer stored as a part of modelingMachine
's context, but rather as a part of appActor
's. Persistence happens as an action alongside setting a new layout. Getters and setters are exported for the rest of the app to make use of, which App.tsx
does.
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.
Note that TestLayout
makes no use of appActor
, providing its own simpler local state management.
import type { Options } from 'react-hotkeys-hook' | ||
import { useHotkeys } from 'react-hotkeys-hook' | ||
|
||
import { codeManager } from '@src/lib/singletons' |
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.
The import of codeManager
here is what forced me to break out hotkeys.ts
into its own file, because I needed the hotkeyDisplay
utility without the circular dependency hell that comes with codeManager
.
function saveLayoutInner({ layout, layoutName = 'default' }: ISaveLayout) { | ||
if (!globalThis.localStorage) { | ||
return | ||
} | ||
globalThis.localStorage.setItem( | ||
`${LAYOUT_PERSIST_PREFIX}${layoutName}`, | ||
globalThis.JSON?.stringify({ | ||
version: 'v1', | ||
layout, | ||
} satisfies LayoutWithMetadata) | ||
) | ||
} | ||
export const saveLayout = throttle(saveLayoutInner, LAYOUT_SAVE_THROTTLE) |
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.
As mentioned in the errata in the PR description, in a future PR I'd like the desktop app to target a config folder on-disk instead of localStorage
, much like our env configs are stored.
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.
This function was used in one place and was briefly frustrating my util tests so I removed it. I believe I fixed what was actually causing the issue due to re-export behavior differences in Vitest, but I decided to leave this change in because we just don't need this code anymore.
This PR implements a system for layout in the modeling scene. Towards #8044, closes #8215.
Previous system
Until now, we have had an adhoc layout “system” accumulated from the initial modeling app prototype. A layout consisted of one modeling scene flanked by a sidebar on either side, which each contained a set of panes and actions. Users could toggle panes opened or closed, and this would be persisted to
localStorage
, but the width of those panes would not be, and vertical resizing between two open panes was not possible. There was certain to be one of each item in the system: one scene, one code pane, one text-to-cad pane, etc.Why we’re doing this
Several of our goals for ZDS require a more general approach:
This PR does not implement any of these but the 6th item, but lays a foundation in preparation for the first 5 and more. Please alert me if there is work in here that jeopardizes those or any other future product goals that touch on layout.
Definitions
areaType
key, and then the application finds the corresponding component from a providedareaLibrary
at runtime.areaType
s. For now, area types are hardcoded and type-checked inareaTypeRegistry.ts
actionType
keys, then looked up at runtime.Note: I removed Tab Layout types, utils, and components from this PR, as we have no use for them yet. I think they could be useful for code editor buffers in future.
What to expect while testing
/layout
route to test more gnarly layout configurations with no other app systems (best reached in the browser)Persistence data structure
The layout is persisted as a JSON string with a
name
andversion
fields. Name is alwaysdefault
for now, leaving open the possibility of multiple user-defined layouts in future. The version is alwaysv1
for now, allowing us to detect breaking changes early. Thelayout
field contains the actual serialized data on the nested layout types as discussed previously. The default layout is in@src/lib/layout/configs/default.ts
. A deeply nested layout definition is available in./configs/test.ts
.Errata
localStorage
, following Nadro’s example from the env work, but felt it was out of scope for this large PR. Future work should honorlocalStorage
and migrate desktop users to an on-disk approach.areaTypeRegistry.ts
andactionTypeRegistry.ts
broke that boundary, so the couple places those are imported are from their files directly instead of from@src/lib/layout
. I could move these somewhere else in the code base I think and have a cleaner divide but I felt like that should wait until we have proper extension-like area and action registration.id
,label
, oricon
items on the Layout data structure. Most Layout nodes probably won’t ever need a human-readable label: really only Pane children that are Simple Layouts need them (for their tooltips), and I already put other metadata such as theshortcut
under the responsibility of theareaTypeRegistry
. Or maybe I have that backwards, and what I should be questioning is ifshortcut
ought to be in the persisted structure in case of multiple of the same area type. TLDR what data goes where—persisted in userland or prescribed by appland—is still a WIP.Demo
Screenshare.-.2025-10-16.5_01_44.PM-compressed.mp4