Feat/configurable page heights#320
Conversation
- Changed fixed height to min-height in editor.html - Removed overflow:hidden to enable scrolling - Updated sidebar and demosidebar to use min-height with max-height - Pages now expand based on content and scroll naturally
…exibility - Changed editor page heights from fixed to flexible (min-height) - Removed overflow:hidden to enable scrolling - Moved "I'm Done" button from fixed position to below right sidebar - Button now scales with page zoom and stays aligned with sidebar - Updated sidebar heights to use min-height with max-height constraints
monicaz010101
requested review from
Copilot and
kcarnold
and removed request for
Copilot
|
This PR should have a Playwright test that shows the problem it's solving, and a screenshot that shows that it's fixed. |
There was a problem hiding this comment.
Pull request overview
Adds configurable, viewport-based page heights to the editor/study UI and relocates the “Done” action into the sidebar layout to avoid fixed positioning.
Changes:
- Update editor/study CSS layouts to use
100vhcontainers with internal scrolling regions. - Extend
EditorScreento accept an optionaldoneButtonand render it beneath the sidebar. - Add Google Docs sidebar bundled artifacts under
frontend/dist-gdocs.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/editor/styles.module.css | Switches containers to viewport-height layouts and updates sidebar/button styling. |
| frontend/src/editor/studyRouter.tsx | Passes the “Done” button into EditorScreen instead of rendering it fixed on the page. |
| frontend/src/editor/index.tsx | Adds doneButton prop and wraps sidebar to support a bottom action area. |
| frontend/src/editor/editor.module.css | Updates sidebar sizing to flex-based layout. |
| frontend/src/editor/editor.html | Adjusts document/body height/overflow defaults to match new scrolling model. |
| frontend/dist-gdocs/sidebar-bundled.html | Adds a bundled HTML entrypoint for Google Docs sidebar (includes dev-loading logic). |
| frontend/dist-gdocs/google-docs.css | Adds bundled CSS output for Google Docs sidebar. |
| frontend/dist-gdocs/google-docs.bundle.js.LICENSE.txt | Adds license attributions for bundled JS dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| height: 100vh; | ||
| overflow: hidden; |
There was a problem hiding this comment.
With the outer container forcing overflow: hidden, the sidebar area must provide its own scrolling when content exceeds the viewport. In this diff the sidebar switches to flex: 1 / min-height: 0 but does not set overflow-y: auto, which can cause sidebar content to be clipped/unreachable in smaller viewports. Fix: add overflow-y: auto (and typically display: flex; flex-direction: column; if needed) to .sidebar/.demosidebar (or remove the parent overflow: hidden and rely on page scrolling).
| flex: 1; | ||
| margin: 20px 0px 20px 0px; | ||
| height: 80vh; | ||
| min-height: 0; |
There was a problem hiding this comment.
With the outer container forcing overflow: hidden, the sidebar area must provide its own scrolling when content exceeds the viewport. In this diff the sidebar switches to flex: 1 / min-height: 0 but does not set overflow-y: auto, which can cause sidebar content to be clipped/unreachable in smaller viewports. Fix: add overflow-y: auto (and typically display: flex; flex-direction: column; if needed) to .sidebar/.demosidebar (or remove the parent overflow: hidden and rely on page scrolling).
| </EditorContext.Provider> | ||
| <div className={`flex flex-col overflow-hidden ${isDemo ? 'w-[28rem] min-w-[28rem] flex-shrink-0 mx-5' : 'flex-[0_1_28rem] min-w-[14rem]'}`}> | ||
| <div | ||
| className={isDemo ? classes.demosidebar : classes.sidebar} |
There was a problem hiding this comment.
The wrapper uses overflow-hidden, but the immediate sidebar container (classes.sidebar/classes.demosidebar) doesn’t obviously enable scrolling. That combination can clip the sidebar contents and/or the new doneButton block when vertical space is tight. Recommendation: keep the wrapper overflow-hidden only if the inner sidebar container enforces scrolling (e.g., add flex-1 overflow-y-auto min-h-0 to the sidebar container), or move the overflow control to the inner sidebar area so the done button remains visible.
| className={isDemo ? classes.demosidebar : classes.sidebar} | |
| className={`flex-1 overflow-y-auto min-h-0 ${isDemo ? classes.demosidebar : classes.sidebar}`} |
| <EditorContext.Provider value={editorAPI}> | ||
| <Sidebar /> | ||
| </EditorContext.Provider> | ||
| <div className={`flex flex-col overflow-hidden ${isDemo ? 'w-[28rem] min-w-[28rem] flex-shrink-0 mx-5' : 'flex-[0_1_28rem] min-w-[14rem]'}`}> |
There was a problem hiding this comment.
The inline className string mixes multiple layout responsibilities with a long conditional tailwind expression, making future tweaks error-prone. Consider extracting these computed class names into a small helper (e.g., sidebarWrapperClass) or using a classnames utility so layout decisions are easier to read and maintain.
The test used aria-label='Delete saved item' but the component renders aria-label='Delete suggestion', causing a 30s timeout on all browsers.
No description provided.