fix(Fullscreen): fix overlay and root #2817
Conversation
| @@ -1,20 +1,22 @@ | |||
| .ydb-fullscreen { | |||
| --ydb-fullscreen-z-index: 1000; | |||
There was a problem hiding this comment.
Fixed a bug with overlay - drawer is above fullscreen
| const fullscreenRoot = React.useMemo( | ||
| () => document.getElementById('fullscreen-root') ?? undefined, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
In dev mode root sometimes was not found - it's undefined, fullscreen doesn't work. Now it should always be present
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Fullscreen component and its dependencies to improve code organization and reliability. The changes convert default exports to named exports and fix potential issues with DOM element access.
Key changes:
- Convert default exports to named exports for Fullscreen and EnableFullscreenButton components
- Move fullscreen root element access inside the component with proper memoization
- Use CSS custom properties for z-index values instead of hardcoded numbers
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Fullscreen/Fullscreen.tsx | Convert to named export, move DOM access inside component with memoization, add fullscreenRoot to effect dependencies |
| src/components/EnableFullscreenButton/EnableFullscreenButton.tsx | Convert default export to named export |
| src/components/Fullscreen/Fullscreen.scss | Replace hardcoded z-index values with CSS custom properties |
| src/containers/Tenant/TenantDrawerHealthcheck.tsx | Update import to use named export |
| src/containers/Tenant/Query/QueryResult/QueryResultViewer.tsx | Update imports to use named exports |
| src/containers/Tenant/Query/Preview/components/PreviewView.tsx | Update imports to use named exports |
| src/containers/Tenant/Healthcheck/Healthcheck.tsx | Update import to use named export |
| src/containers/Tenant/Diagnostics/TopicData/TopicData.tsx | Update imports to use named exports |
artemmufazalov
requested review from
Raubzeug,
adameat and
astandrik
as code owners
| const isFullscreen = useTypedSelector((state) => state.fullscreen); | ||
| const dispatch = useTypedDispatch(); | ||
|
|
||
| const fullscreenRoot = React.useMemo( |
There was a problem hiding this comment.
Could we use ref instead of low-level dom manipulations?
Cursor suggests two options for this
There was a problem hiding this comment.
Added ref and context
Stand: https://nda.ya.ru/t/rNCLFY_v7JT8iT
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 85.39 MB | Main: 85.39 MB
Diff: +2.13 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information