fix: make asset preview overlay z-index configurable#451
Conversation
Deploying ui2 with
|
| Latest commit: |
3ade991
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a6d98ed0.ui2-423.pages.dev |
| Branch Preview URL: | https://fix-asset-preview-overlay-z.ui2-423.pages.dev |
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — fix: make asset preview overlay z-index configurable
Branch: fix/asset-preview-overlay-z-index → master
Files changed: 3 (+15 −5)
Summary
Clean, minimal change that exposes the hardcoded z-index: 1600 on the portaled PlayerOverlay as an optional overlayZIndex prop on AssetPreviewPlayerProvider. The default is preserved via nullish coalescing (??), so all existing consumers keep the current behavior with zero changes. Consumers that render on a bare page behind a fixed navbar can now pass a lower value to keep the preview under the navbar.
Findings
No P0 or P1 issues found.
[P2 — Minor] No test coverage for the new prop
There don't appear to be unit tests for this component currently, so this is a pre-existing gap rather than a regression. Worth considering in a follow-up, but not blocking.
Detailed Analysis
- TypeScript —
overlayZIndex?: numberis the correct type. Properly threaded through the styled component's generic parameter.??is the right operator (handles bothundefinedandnull). ✅ - Architecture — Simplest possible solution. The z-index conflict (navbar 1100 < dialog 1300 < preview 1600) genuinely can't be solved with a single value, and exposing it as a prop follows the existing pattern of optional configuration in this component (
peerUrl,marketplaceServerUrl,profile,dev). ✅ - Consumer impact — The package is
decentraland-ui2. The only external consumer ofAssetPreviewPlayerProviderisdecentraland/sites(profileCreationsTab.tsx,AssetsTab.tsx). This is a purely additive change (new optional prop); existing call sites are unaffected. ✅ - JSDoc — Excellent documentation on the new prop explaining the default, why it exists, and when to use a different value. ✅
- Git conventions (ADR-6) — PR title follows
<type>: <summary>format; branch follows<type>/<summary>pattern. ✅ - Security — No concerns. The prop is a numeric CSS value flowing through Emotion's object-style API (no template literal injection surface). No auth, input handling, or data exposure changes. ✅
Verdict
Approved — no blockers. Well-scoped fix with clear documentation and zero impact on existing consumers.
Reviewed by Jarvis 🤖 · Requested by Braian Mellor via Slack
443bbd4 to
3ade991
Compare
The shared asset-preview overlay is portaled to
document.bodywith a fixedz-indexso a hovered card's live preview floats over the card. It was hardcoded to1600, which sits above every MUI chrome layer (appBar1100,modal1300,snackbar1400,tooltip1500) — so on a bare page the preview renders over the fixed navbar, and in general it can cover dialogs/toasts it shouldn't.This exposes the overlay z-index as an optional
overlayZIndexprop onAssetPreviewPlayerProviderand lowers the default to1050— above page content but below app chrome — so the preview no longer covers the navbar, modals or toasts by default. The one case that needs to float above a modal (cards hovered inside a dialog) opts in by passing a higher value (e.g.1600).Verified with
npm run build(tsc) and lint.