fix(frontend): make bulk-review env list scrollable above max-height (#736)#737
Open
nanookclaw wants to merge 1 commit into
Open
fix(frontend): make bulk-review env list scrollable above max-height (#736)#737nanookclaw wants to merge 1 commit into
nanookclaw wants to merge 1 commit into
Conversation
…anonical#736) The bulk-review dialog rendered every selected environment in an unbounded Column inside the AlertDialog. With a large selection (e.g. 129 envs at 4k/100% scale, 50% browser zoom in the report) the list overflowed the dialog/viewport and was unreachable without zooming the browser out — there was no scroll affordance and the action buttons sat below the overflow. Wrap the env list in a Scrollbar+SingleChildScrollView with BoxConstraints(maxHeight: 240) so the list scrolls in place once it exceeds the cap. Bordered container, dividers, and the rest of the dialog layout are unchanged. 240px is roughly 10-11 rows at the existing bodySmall + level1 vertical padding, matching the maintainer comment in the issue ("scrollable container of sorts beyond a maximum height"). Signed-off-by: Nanook Claw <[email protected]>
There was a problem hiding this comment.
Pull request overview
Fixes the Bulk Environment Review dialog UI overflow by constraining the selected-environments list and making it scrollable so the action buttons remain reachable for large selections (closes #736).
Changes:
- Constrains the environments list container to a maximum height.
- Wraps the environments list with
Scrollbar+SingleChildScrollViewto enable in-place scrolling.
Comment on lines
+115
to
+121
| constraints: const BoxConstraints(maxHeight: 240), | ||
| child: Scrollbar( | ||
| thumbVisibility: true, | ||
| child: SingleChildScrollView( | ||
| child: Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ |
Comment on lines
+116
to
+119
| child: Scrollbar( | ||
| thumbVisibility: true, | ||
| child: SingleChildScrollView( | ||
| child: Column( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #736.
Problem
BulkEnvironmentReviewDialogrenders every selected environment in an unboundedColumninside theAlertDialog. With a large selection (e.g. 129 envs at 4k/100% scale, 50% browser zoom per the reporter's screenshot) the env list overflows the dialog and viewport — there is no scroll affordance and the Cancel / Submit Reviews buttons end up below the overflow, so the dialog is effectively unusable without zooming the browser out.Fix
Wrap the env list in
Scrollbar+SingleChildScrollViewand cap the container atmaxHeight: 240. Once the list exceeds the cap it scrolls in place; everything else (border, dividers, decision checkboxes, comment field, action buttons) is untouched.240px is roughly 10-11 rows at the existing
bodySmalltext +Spacing.level1vertical padding, which matchesmotjuste's comment on the issue ("scrollable container of sorts beyond a maximum height").Notes
Columnstill usesmainAxisSize: MainAxisSize.minand the constraint only kicks in once content exceeds the cap.frontend/test/ui/has noartefact_page/), so this PR adds none. Happy to add one if you'd like a separate harness PR.