Rework screenspace renderable panel - #233
Conversation
Also create a separate component to be used for building add icons with a consistent look
…e-renderable-update
…subowners in the renderable tab
# Conflicts: # src/components/DecoratedIcon/DecoratedAddIcon.tsx # src/components/DecoratedIcon/DecoratedIcon.tsx # src/panels/GlobeImageryBrowserPanel/CapabilityEntry.tsx # src/panels/GlobeImageryBrowserPanel/GlobeImageryBrowserPanel.tsx
|
Suggestion to make the "Tip: Try adding by drag-and-dropping..." a bit smaller font-size and just a bit darker grey to keep the focus on the "Add screen pace renderables" |
|
The "Roll" in the Local Rotation behaves opposite to how I would have expected it. Moving the widget clockwise makes the screenspace renderable rotate counter-clockwise |
| "label": "Azimuth", | ||
| "aria-label": "Azimuth" |
There was a problem hiding this comment.
Just out of curiosity. Is an aria-label necessary if it is the same content as the label? Or is it best practice to add it anyway?
There was a problem hiding this comment.
It's not if the label is part of the UI component. An example is the Button component. In this case, the text is part of the actual component and will be read out by a screen reader when the button is focused.
In this case, the "label" is just a text above the AngleSlider component, which means that the screen reader will not read the label when focusing the actual slider. Thus the need for an aria-label :)
Good-to-know: Mantine has per-component recommendations for accessibility: https://mantine.dev/core/angle-slider/#accessibility
There was a problem hiding this comment.
It is possible to use another component for the aria-label, however, to avoid repetition:
This might actually be a good approach in this case - I'll update it
|
Works great! And so much better than before! |
Co-authored-by: Alexander Bock <mail@alexanderbock.eu>
There was a problem hiding this comment.
Pull request overview
This PR reworks the ScreenSpace Renderable panel UI to align with the newer property-owner structure, moving from the old “tab-based add + inline owners” layout to a scene-menu style list + selection details view with improved placement controls.
Changes:
- Replaces the old Image/Webpage tabs with an “Add” modal supporting image, webpage, video, and text renderables.
- Introduces a list + selection-based renderable view, including a “more” menu (pop-out/delete) and type icons with tooltips.
- Adds custom placement UI (Cartesian vs RAE, local rotation) and a small global NumberInput theme tweak.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/removeModalsHooks.tsx | Adds a reusable confirm-modal hook for deleting screenspace renderables |
| src/theme/mantineTheme.ts | Adds default NumberInput hold-to-step behavior |
| src/panels/ScreenSpaceRenderablePanel/WebpageTab.tsx | Removes legacy webpage add tab |
| src/panels/ScreenSpaceRenderablePanel/ImageTab.tsx | Removes legacy image add tab |
| src/panels/ScreenSpaceRenderablePanel/TypeIcon.tsx | Adds type-to-icon mapping with tooltip labels |
| src/panels/ScreenSpaceRenderablePanel/ScreenSpaceRenderablePanel.tsx | New list/search + selected-details panel layout |
| src/panels/ScreenSpaceRenderablePanel/ScreenSpaceRenderableView.tsx | Adds detailed view with tabs for renderable/placement/style |
| src/panels/ScreenSpaceRenderablePanel/ScreenSpaceRenderableMoreMenu.tsx | Adds pop-out/delete menu for a renderable |
| src/panels/ScreenSpaceRenderablePanel/ScreenSpaceRenderableListItem.tsx | Adds list row UI with visibility toggle, type icon, and more menu |
| src/panels/ScreenSpaceRenderablePanel/Add/AddModal.tsx | New modal UI for adding renderables |
| src/panels/ScreenSpaceRenderablePanel/Add/hooks.ts | Adds add-renderable helpers (image/web/video/text) |
| src/panels/ScreenSpaceRenderablePanel/Placement/* | New custom placement controls and property grouping UI |
| src/panels/Scene/SceneGraphNode/SceneGraphNodeMoreMenu.tsx | Updates remove hook import path |
| src/panels/GeoLocationPanel/AddedCustomNodes.tsx | Updates remove hook import path |
| src/icons/icons.tsx | Adds new exported icons used by the panel/controls |
| src/hooks/userLevel.ts | Adds a hook to detect “advanced user” level |
| src/components/PropertyOwner/PropertyOwner.tsx | Extends hideSubowners to support hiding specific subowners |
| src/components/AngleInput/AngleInput.tsx | Adds a combined angle slider + numeric input control |
| public/locales/en/panel-screenspacerenderable.json | Updates translations for new UI structure and text |
Suppressed comments (2)
src/panels/ScreenSpaceRenderablePanel/Add/hooks.ts:96
- Identifier fallback uses
slideURLwhenmakeIdentifier(orluaApi) is unavailable. This can produce invalid/very long identifiers and is inconsistent with other add helpers (e.g.,addTextfalls back toname).
async function addWebpage(name: string, slideURL: string) {
const osIdentifier = (await luaApi?.makeIdentifier(name)) ?? slideURL;
src/panels/ScreenSpaceRenderablePanel/Add/hooks.ts:113
- Identifier fallback uses
slideURLwhenmakeIdentifier(orluaApi) is unavailable. Using the URL/path as an identifier can easily create invalid identifiers (and can be extremely long).
) {
const osIdentifier = (await luaApi?.makeIdentifier(name)) ?? slideURL;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| matcherFunc={(uri: Uri, searchString: string) => | ||
| propertyOwners[uri]?.name | ||
| .toLowerCase() | ||
| .includes(searchString.toLowerCase()) | ||
| } |
| async function addImage(name: string, slideURL: string) { | ||
| const osIdentifier = (await luaApi?.makeIdentifier(name)) ?? slideURL; | ||
|
|
| if (userLevel === undefined) { | ||
| throw new Error('User level property is undefined'); | ||
| } |
| function getFileNameFromUrl(data: string) { | ||
| const urlParts = data.split('/'); | ||
| const lastPart = urlParts[urlParts.length - 1]; | ||
| return lastPart.split('.')[0]; // Remove file extension if present | ||
| } |
Based on the new property owner structure in this OpenSpace PR: OpenSpace/OpenSpace#4204
Please try it out and let me know what you think, and if anything is missing or not working as expected.
Summary of features and new visual look below
Before any renderables exist:
Adding is now done in a separate modal
And now includes the option to add videos from local files as well as screenspace texts
Icons for added types:
Almost all types have an assigned icon
With details on hover:
Search by name:
Scene-menu style selection and display of data:
Select an item in the list to view its details at the bottom
Custom input components for placement:
Z and radius is only shown for advanced users. For normal users:
This is communicated through a custom property description:
Removal and pop-out are done in "..."-menu
Features left for future work: