refactor(spx-gui): simplify modal transform-origin handling & fix select trigger background#3113
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the UIModal component to manage its transform origin via a CSS variable and simplifies the underlying logic. It also updates ID generation across several utilities and adjusts styling priorities in UISelect. Feedback highlights a potential bug where the CSS variable is not cleared when the origin is null, a regression in UIModal regarding the priority of custom origins, and a styling regression in UISelect where validation states override user interaction feedback.
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the spx-gui UI infrastructure to simplify modal transform-origin handling, standardize layer/modal ID allocation semantics, and adjust UISelect styling for validation states.
Changes:
- Simplified
UIModaltransform-origin handling by switching to a CSS custom property (--ui-modal-transform-origin) instead of recomputing/binding inline styles. - Updated modal/layer stack ID counters to “next-id” semantics (use current
nextId, then increment), starting from1. - Tweaked
UISelectbackground styling when insuccess/errorvalidation states.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spx-gui/src/components/ui/utils/layer-stack.ts | Aligns layer registration ID allocation to next-id semantics. |
| spx-gui/src/components/ui/select/UISelect.vue | Adds a background rule for success/error validation states. |
| spx-gui/src/components/ui/modal/UIModalProvider.vue | Aligns modal instance ID allocation to next-id semantics. |
| spx-gui/src/components/ui/modal/UIModal.vue | Refactors transform-origin handling to use a CSS variable on the modal surface. |
| spx-gui/src/components/ui/modal/UIModal.test.ts | Updates animation-origin assertions to match the CSS-variable approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The CSS custom property approach for transform-origin is clean, and the id counter semantics fix is correct. Two issues worth addressing:
Stale layout read (medium): Removing flush: 'post' / await nextTick() from the second watcher means offsetLeft/offsetTop may be read before the teleported surface has been laid out by the browser. The original code included an explicit comment explaining this requirement. On first open the offset values could be 0, making the computed origin equal to raw viewport coordinates instead of element-relative coordinates, producing a visually wrong scale animation entry point. The test suite passes because await flushModal() calls several nextTick rounds before asserting — in JSDOM layout is synchronous, so it works there but not in a real browser.
CSS focus state suppressed for validated selects (minor): The new rule .ui-select:is([data-ui-state='success'], [data-ui-state='error']):not(:hover) has equal specificity to the earlier :focus rule and appears later in the stylesheet, so it wins when the element is focused-but-not-hovered — suppressing the focused background appearance on validated selects.
Changes