-
-
Notifications
You must be signed in to change notification settings - Fork 727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Adjust scroll from clipping children form fields in AlertDialog
from showSourcesDialog
#1782
Conversation
This commit resolves the issue of children clipping in `AlertDialog` for the `showSourcesDialog` by adjusting the structure to manage scrolling behavior more efficiently. By doing this, the user experience and readability is greatly improved on small devices and/or devices with large fonts. - Remove child SingleChildScrollView - Add parent (dialog) scrollable property to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance it LGTM (Looks Good To Me), nothing screwed up so far...
⚒️ Build status✅ Succeeded build on 22272c3. Details: 8404775438! ⚙️ Workflow run configuration
|
@@ -27,6 +27,7 @@ class SManageSources extends BaseViewModel { | |||
return showDialog( | |||
context: context, | |||
builder: (context) => AlertDialog( | |||
scrollable: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the previous AlertDialog title was fixed in-place and the modified one is scrollable along the content with this line, as the title
and content
are wrapped with the scrollable
.
It's not that big of an issue, it looks natural (even only noticed after attaching the docs & refs) but it's the cleanest way to fully address this issue on all devices.
🔨 Break the glass in case of concern:
If this is unwanted, the 'hacky' alternatives would be to add extra padding or size with a
SizedBox
orPadding
having a magic number that could not work on some specificMediaQuery's
, virtual keyboards, or screen sizes.
scroll-sample.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a plus because, after #1791, the title can now expand lines (grow in the Y axis [vertical] ) when it's too large to fit instead of clipping. This would make the [AlertDialog]
content smaller and smaller (improbable but still possible), which at some point would be very hard to scroll and interact.
However, if the title and content are scrollable, this wouldn't be the case, as the user could easily scroll and focus on the content after a multi-line title.
Note to teamCaution DO NOT use the PR builder tool, this is affected by
|
AlertDialog
from showSourcesDialog
(#1748)AlertDialog
from showSourcesDialog
(#1748)
@validcube can you review this? |
AlertDialog
from showSourcesDialog
(#1748)AlertDialog
from showSourcesDialog
(#1748)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥞 Approved, thanks!
Test result (🥞 Skipped)
- Only analysis check is done, this PR is small and contains self-testing video with the fixed behaviour applied.
I shouldn't seen that first... 💀 Sorry.
AlertDialog
from showSourcesDialog
(#1748)AlertDialog
from showSourcesDialog
# [1.21.0-dev.5](v1.21.0-dev.4...v1.21.0-dev.5) (2024-06-26) ### Bug Fixes * Adjust scroll from clipping children form fields in `AlertDialog` from `showSourcesDialog` ([#1782](#1782)) ([bbeb836](bbeb836))
# [1.21.0](v1.20.1...v1.21.0) (2024-07-29) ### Bug Fixes * Add missing import to patch options field ([d60f9aa](d60f9aa)) * Adjust scroll from clipping children form fields in `AlertDialog` from `showSourcesDialog` ([#1782](#1782)) ([bbeb836](bbeb836)) * Cache external API calls ([#1911](#1911)) ([2c3e2e6](2c3e2e6)) * Change problematic translation string ([6b03f3a](6b03f3a)) * Correct architecture to armeabi-v7a ([63c6412](63c6412)) * Download latest integrations non-pre-release ([4a72267](4a72267)) * Follow language update immediately ([#1944](#1944)) ([c13827e](c13827e)) * Follow system theme immediately ([#1942](#1942)) ([694f2a9](694f2a9)) * Handle selecting files and folders for patch options correctly ([#1941](#1941)) ([b26760b](b26760b)) * Increase dashboard RefreshIndicator edge offset ([#1859](#1859)) ([232b702](232b702)) * Patching Screen draw-behind Navigation Bar ([#1945](#1945)) ([f1b25d0](f1b25d0)) * Restore consistency with the app ([ea9654e](ea9654e)) * SecurityException when patching application ([#1856](#1856)) ([e0a6de2](e0a6de2)) * Select previously applied patches when loading patch selection ([#1865](#1865)) ([7ef8f04](7ef8f04)) * Unable to install application regardless of preference ([c7627ce](c7627ce)) * Unsupported patch toast says "patchItem.unsupportedPatchVersion" ([#2011](#2011)) ([3209c0e](3209c0e)) * Update dialog shows dev version & loading gets stuck in certain circumstances ([#1792](#1792)) ([fc52560](fc52560)) ### Features * Add ability to set `null` in patch options ([#1947](#1947)) ([5c68d51](5c68d51)) * Include primary architecture in external search ([#2068](#2068)) ([23690a9](23690a9)) * open browser when clicking on changelog link ([bc300d8](bc300d8)) * Save last patched app ([#1414](#1414)) ([7720408](7720408)) * Support patching on ARMv7a ([a766352](a766352))
🔧🎨 UI Fix: Adjust Scroll from clipping it's children form fields in the
AlertDialog
fromshowSourcesDialog
(#1748)This PR closes #1748
Overview
The
AlertDialog
's scrolling behavior forshowSourcesDialog
was causing children to clip, affecting user experience and readability, especially on devices with small screens or large fonts.AlertDialog
.📓 Description
This PR addresses the issue of children clipping within the
AlertDialog
by adjusting the structure to handle scrolling more efficiently. It removes unnecessary scroll components and enables the builtin scroll handle from the FlutterAlertDialog
, ensuring content within the dialog is properly displayed without clipping.✍🏼 Changes Made
SingleChildScrollView
.🦯 Testing
Update Tests (ToDo - Not Found nor needed for now...)Stable Flutter v3.19.3
anddart 3.3.1
.📋 Notes & References
🔬 Reviewers
N/A (any mod is ok)
👾 Feedback, changes, or suggestions are welcome! ✌🏼 🏖️ now a 🍕 emoji too...