fix: make toolbar responsive without narrow mode - #3023
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Layout containment changes the containing block for existing fixed-position overlays, breaking viewport-based positioning.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces JavaScript narrow-mode detection with responsive CSS container queries.
Changes:
- Removes the synchronous
offsetWidthcheck. - Adds container-query styling for toolbar, toolbox, and RTL layouts.
- Adds Cypress regression coverage for initialization and holder resizing.
File summaries
| File | Description |
|---|---|
src/components/modules/ui.ts |
Removes narrow-mode detection and class. |
src/styles/ui.css |
Defines the container and responsive layout. |
src/styles/toolbar.css |
Repositions the plus button. |
src/styles/toolbox.css |
Aligns the narrow toolbox. |
src/styles/rtl.css |
Adds responsive RTL positioning. |
src/styles/variables.css |
Renames the toolbar spacing variable. |
test/cypress/tests/modules/Ui.cy.ts |
Adds responsive-layout regressions. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * Editor wrapper | ||
| */ | ||
| .codex-editor { | ||
| container: editor / inline-size; |
There was a problem hiding this comment.
Fixed in 1172deb.\n\nI reproduced the containment regression against the real rectangle-selection overlay before changing the implementation: after offsetting the holder, .codex-editor-overlay started at x=49 instead of the viewport's x=0.\n\nThe CSS size container is now removed. A ResizeObserver toggles the responsive codex-editor--compact class from the delivered wrapper size, so holder resizes remain responsive without a synchronous offsetWidth read or a new containing block for fixed descendants.\n\nA Cypress regression now pins the overlay to the 1000 x 800 viewport. Verification: focused UI 7/7, full Cypress 354/354 across 40 specs, lint and zero-error lint passed, production build passed, and git diff --check passed. The known Flipper test retried once before passing.
Replace layout containment with a ResizeObserver-driven responsive class so fixed overlays keep viewport coordinates without restoring the synchronous width read.
| } | ||
|
|
||
| this.nodes.wrapper.classList.toggle( | ||
| this.CSS.editorWrapperCompact, |
There was a problem hiding this comment.
You're simply renamed "narrow" to "compact". Consider removing this mode at all and make the responsive layout instead. The Plus should always stay left, even on thin containers.
Note 1. Screen may be wide, but the container holding the editor is thin. Note 2. Make sure, the mobile version works as before.
There was a problem hiding this comment.
Got it, that makes a lot of sense.
I’ve removed the hardcoded compact mode and transitioned to a fully responsive layout. The plus button is now pinned to the left even in narrow containers, and I verified that the mobile view still behaves as expected.
Ready for another look whenever you have time!
There was a problem hiding this comment.
Addressed in bc2dcb1. I removed the mode entirely: there is no narrow/compact class, holder-width read, or ResizeObserver now.
The toolbar and block content use a continuous CSS layout. On a 1000 px screen, resizing the editor holder to 500 px keeps the Plus button at the holder's leading edge and leaves the content after the action rail. I also added the mirrored RTL case explicitly because this repo's PostCSS build resolves logical properties to physical LTR properties.
Mobile keeps the existing toolbar and bottom-sheet toolbox behavior, and the fixed viewport overlay regression remains covered. Local verification is green: focused 9/9 and full Cypress 356/356 across 40 specs, plus lint and production build.
Could you please take another look?
|
@neSpecc The requested responsive-layout follow-up is available in It removes the Narrow/Compact runtime mode, holder-width decision, and Fresh local verification on this exact SHA with Node 18.20.1: focused The prior review threads are now outdated, while the Cypress and ESLint workflow runs remain |
Fixes #3022
What changed
ResizeObserver, ornarrow/compactmodifier classThe layout reacts directly to holder-size changes through CSS; it does not depend on a window resize event or a JavaScript breakpoint mode.
Reproduction
Before
bc2dcb1, a 500 px holder in a 1000 px viewport put the Plus button at x=483 (the holder's right side) instead of its leading content edge at x=9. The new regression checks first failed on that geometry and now pass with the Plus button at the holder's leading edge and the block content after the action rail.An RTL regression check also caught that this repository's PostCSS target resolves logical properties to physical LTR properties at build time. The final CSS therefore contains an explicit RTL mirror, and the compiled behavior is covered rather than inferred from source syntax.
Local verification
Ui.cy.ts: 9/9 passedyarn test:e2e: 356/356 passed across 40 specsyarn lint: exit 0 (existing repository warnings only; no errors)yarn lint:errors: exit 0yarn build: exit 0git diff --check: exit 0The full Cypress run completed without a process crash. Cypress retried one pre-existing
utils/flipper.cy.tscase once; it passed on retry and is outside this sizing path.