Skip to content

fix: Add staticContent to SmoothModalSheet and constrain navbar reord… #6482

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

Closed

Conversation

TapItNinja
Copy link

Fixes navbar reordering crash (#6451) due to unbounded height post-#6443:

  • Added staticContent to SmoothModalSheet (@monsieurtanuki’s suggestion).
  • Capped _computeContentSize at 80% screen height.
  • Replaced SmoothAnimatedList with ListView.separated (shrinkWrap: true).

Tested across devices—no errors. Balances #6443’s fix with functionality. Open to reverting #6443 if @g123k prefers!

Fixes #6451, updates #6462.

Simulator.Screen.Recording.-.iPhone.16.-.2025-03-27.at.16.22.45.mp4

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TapItNinja!

Please

  • revert what was coded in #6443 first, in order to fix #6451 (and check it does work)
  • on top of that, fix again #6441 the way it was coded but without impacting #6451 - as discussed before, just adding a bool parameter that would make the navbar work exactly as it used to work and the static content exactly as coded in #6443

@@ -60,12 +64,17 @@ class ProductFooterSettingsButton extends StatelessWidget {
/// Header + list padding + for each action: height + separator
/// + bottom padding (nav bar)
double _computeContentSize(BuildContext context) {
return SmoothModalSheetHeader.MIN_HEIGHT +
final double availableHeight =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep your comments to the very very very minimum, in general for this PR.

@@ -49,8 +49,12 @@ class ProductFooterSettingsButton extends StatelessWidget {
title: appLocalizations.product_page_action_bar_setting_modal_title,
prefixIndicator: true,
closeButton: true,
expandBody: true,
body: const _ProductActionBarModal(),
expandBody: false, // Avoid unbounded expansion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I were you I wouldn't recode this part with your personal tastes.
The point here is not to change the way it used to be, the point is to put it how it was before.
The only thing you should do here is say "btw it's dynamic content" (or "not static", whatever), with an additional parameter.

@@ -318,6 +318,7 @@ class SmoothModalSheet extends StatelessWidget {
Color? headerForegroundColor,
this.bodyPadding,
this.expandBody = false,
this.staticContent = false, // New parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come is it false by default?
Most of the time it's stupid static content like "blablabla click-ok", isn't it?

@teolemon teolemon requested a review from Copilot May 16, 2025 19:47
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the navbar reordering crash (#6451) by refining modal layout behavior and content sizing. The key changes include:

  • Adjusting the product footer settings modal to use a fixed height container and disabling automatic expansion.
  • Capping the computed content size at 80% of the screen height.
  • Replacing SmoothAnimatedList with ListView.separated for a more predictable layout.
  • Introducing a new staticContent parameter in SmoothModalSheet and modifying its layout wrapping logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/smooth_app/lib/pages/product/product_page/footer/new_product_footer_settings.dart Updated modal layout: fixed SizedBox for body and revised content size calculation
packages/smooth_app/lib/generic_lib/bottom_sheets/smooth_bottom_sheet.dart Added staticContent parameter and refactored body wrapping conditions
Comments suppressed due to low confidence (1)

packages/smooth_app/lib/pages/product/product_page/footer/new_product_footer_settings.dart:52

  • Consider verifying that disabling expandBody while using a SizedBox for the body is consistently aligned with the intended modal behavior across different screen sizes. Additionally, ensure that the 'size' variable used on line 55 is clearly defined in the surrounding context.
expandBody: false, // Avoid unbounded expansion

@@ -348,8 +350,10 @@ class SmoothModalSheet extends StatelessWidget {
child: body,
);

if (expandBody) {
bodyChild = Expanded(child: bodyChild);
if (expandBody && !staticContent) {
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional logic that wraps the body in a scroll view based on the staticContent flag could benefit from a clearer structure. Consider refactoring this logic to make the separation of dynamic versus static content behavior more explicit.

Copilot uses AI. Check for mistakes.

@teolemon teolemon closed this Jun 12, 2025
@github-project-automation github-project-automation bot moved this from 💬 To discuss and validate to 🎊 Done in 🤳🥫 The Open Food Facts mobile app (Android & iOS) Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

The reordering system in the navbar is broken
3 participants