Fix assessments tab height - #6111
Conversation
🟡 Waiting for changesLast updated: 2026-08-27 11:27 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6111 — replacing the calc(100vh - 128px) chain with a percentage-height chain is the right direction, and the chain does resolve: VCard → .uploader-container → .v-content → .v-content__wrap → .wrapper → .edit-view-layout → .tab-content, which picks up flex: 1 1 100% from Vuetify's global .container rule (shared/styles/vuetify.scss:7634). CI passing.
- suggestion —
Uploader.vue: shared component getsheight: 100%for one consumer's benefit (inline) - suggestion —
EditModal.vue:64pxduplicatesBottomBar's height with nothing linking them;!importantunexplained; comment ends mid-sentence (inline) - nitpick ×2 — dead
max-height: inherit, possibly redundant wrapper div (inline)
Manual QA did not run on this branch, so nothing here is visually verified. The one case worth exercising before merge is #5938's exact repro — Questions tab, a single collapsed question, tall viewport — since the screenshot in the description shows a long list that fills the column regardless. Worth also confirming the Details tab still scrolls with sticky tab elevation, since .wrapper's bounds now rest entirely on the new chain.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| <style lang="scss" scoped> | ||
|
|
||
| .uploader-container { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Uploader has six consumers (FileUploadItem, SupplementaryItem, SupplementaryList, ContentNodeThumbnail, ChannelThumbnail, EditModal); only EditModal needs the full-height root. The others resolve to auto today, but any future definite-height or flex parent will stretch them silently.
Vue 2 forwards a class on a component tag to the child's root, and the repo already ships a global .fill-height { height: 100% } (shared/styles/vuetify.scss:7835) — so <Uploader class="fill-height" …> in EditModal.vue:15 gets the same result with the blast radius down to one screen.
|
|
||
| &.has-bottom-bar { | ||
| // Reserve room for the fixed BottomBar, | ||
| padding-bottom: 64px !important; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: BottomBar.vue:47 hardcodes height: 64px; this restates it in another file with nothing tying the two together — change the bar and content is clipped or a gap appears, with no test or compile signal. A shared SCSS variable both files import would keep them in lockstep.
The !important is load-bearing and non-obvious: VContent writes a padding shorthand inline from Vuetify's app registry (BottomBar is a plain position: fixed div, not app-registered, so the registry doesn't account for it). That's the part the comment should explain — and it currently ends mid-sentence: // Reserve room for the fixed BottomBar,
There was a problem hiding this comment.
Just created the shared module; I don't want to overshare Vuetify details because we will remove it soon, so those comments will get stale and will be hard to update or remove.
| .wrapper { | ||
| min-width: 100%; | ||
| height: 100%; | ||
| max-height: inherit; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: max-height: inherit is now dead — it inherited from ::v-deep .v-content__wrap { max-height: calc(100vh - 128px) }, which this PR removes. .v-content__wrap computes to none, so this resolves to none. Worth deleting alongside the rule it depended on.
| z-index: 5; | ||
| } | ||
|
|
||
| .edit-view-layout-wrapper { |
There was a problem hiding this comment.
nitpick: Is the extra div doing something? .wrapper already carries a definite height: 100%, so .edit-view-layout's min-height: 100% should resolve against it directly without the intermediate wrapper.
There was a problem hiding this comment.
Yes, it is preventing this PR from getting an unnecessarily large diff; it's on purpose.
| height: 100%; | ||
| } | ||
|
|
||
| .edit-view-layout { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Plain flex column in a <style> block instead of VLayout/VFlex is exactly what AGENTS.md asks for, and kept in-scope rather than sweeping the file.
3a4e3c9 to
cd59ec8
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6111 — 3 of 5 prior findings resolved; 2 still open, both contested and deferred to you.
EditModal.vue:703—!importantleft unexplained. You declined on the grounds that documenting Vuetify internals will go stale once Vuetify is removed. Reasonable; not pursuing.EditView.vue:448— possibly redundant.edit-view-layout-wrapperdiv. Kept deliberately to hold the diff down. Reasonable; not pursuing.
Re-traced the percentage-height chain against the amended code — no new correctness issues. CI passing. Manual QA did not run on this branch, so nothing here is visually verified; COMMENT rather than approve for that reason alone.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/files/Uploader.vue:286 — shared component given height: 100% for one consumer
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue:703 — 64px duplicated BottomBar height with nothing linking them
RESOLVED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditView.vue:497 — dead max-height: inherit
CONTESTED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue:703 — !important left unexplained in the comment
CONTESTED — contentcuration/contentcuration/frontend/channelEdit/components/edit/EditView.vue:448 — possibly redundant .edit-view-layout-wrapper div
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| // Height of the fixed BottomBar. Content that sits behind the bar reserves the | ||
| // same amount of room for it. | ||
| $bottom-bar-height: 64px; |
There was a problem hiding this comment.
praise: Defined once and consumed by BottomBar.vue itself, not just by the component that reserves space for it — that's what keeps the two in lockstep.
2 participants
Summary
References
Closes #5938.
Reviewer guidance
questionstab.AI usage
I made the changes, as it was a very opinionated frontend change, with Claude explaining what some Vuetify components do.