feat(data-weaver): adjust z-indexes and overlay positioning - #445
feat(data-weaver): adjust z-indexes and overlay positioning#445pablonoel wants to merge 3 commits into
Conversation
- Lift UI overlays above page content: switch controls/selection containers to fixed positioning and move Selection into its own Portal. - Add $z-index-prompts and $z-index-menus and apply them to controls, menus, selection and prompts. - Small UI tweaks: increase prompt border-radius. - Update color tokens by adding surface-faded and surface-strong, and change atlas-surface and card-surface to use the new tokens. - Changes touch in_front_of_canvas components, page_home styles, _z-indices, and tokens/colors.json.
![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 layout, layering, and styling of the canvas overlays. It splits the portal container into separate portals for controls/tools and selection, introduces new z-index levels ($z-index-prompts and $z-index-menus), and updates several color tokens. Feedback on these changes identifies a stacking context issue where the .selection-container's z-index limits its descendants, causing action menus to render behind prompts. It is recommended to increase .selection-container's z-index to $z-index-menus to resolve this.
| .selection-container { | ||
| position: fixed; | ||
| inset: 0; | ||
| z-index: $z-index-above-content; |
There was a problem hiding this comment.
Because .selection-container has position: fixed, it creates a new stacking context. Setting its z-index to $z-index-above-content (which is lower than $z-index-prompts) caps the stacking level of all its descendants. As a result, even though .actions-container inside the selection has z-index: $z-index-menus, it will render behind the prompts (such as the AI-thinking-Card or prompt input) which are at $z-index-prompts. To ensure the selection and its action menus correctly render on top of the prompts as intended, the .selection-container's z-index should be increased to $z-index-menus.
| z-index: $z-index-above-content; | |
| z-index: $z-index-menus; |
There was a problem hiding this comment.
hi pablo - please fix if this makes sense
nick-nlb
changed the title
beets
left a comment
beets
left a comment
There was a problem hiding this comment.
thank you!
this might conflict with the other z-index change, so will let you merge after you've had a chance to take a look
| .selection-container { | ||
| position: fixed; | ||
| inset: 0; | ||
| z-index: $z-index-above-content; |
There was a problem hiding this comment.
hi pablo - please fix if this makes sense
|
This is closed as superseded by a subsequent PR (now already merged in). |
3 participants
Overview
This PR address the following
Related Issues
Fixes # (issue number)
Changes Made
Testing Done
Describe the steps you took to test these changes (please also list commands ran if possible).
Checklist
Note: Only Maintainers can approve and merge PRs. Expected initial review time: 3 business days.