Skip to content

Conversation

DMouayad
Copy link
Collaborator

@DMouayad DMouayad commented Oct 20, 2025

closes #515

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read and followed the Flutter Style Guide.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making.
  • I followed the Data Driven Fixes where supported.
  • All existing and new tests are passing.
  • I bumped the package version following the Semantic Versioning guidelines (For now the major is the second number and the minor the third, because the package is not feature complete). For example, if the package is at version 0.18.0 and you introduced a breaking change or a new feature, bump it to 0.19.0, if you just added a fix or a chore bump it to 0.18.1.
  • I updated the CHANGELOG.md file with a summary of changes made following the format already used.

If you need help, consider asking for advice on Discord.

Summary by CodeRabbit

  • Bug Fixes
    • Improved dialog widget layout and sizing behavior for better presentation of content.

Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Walkthrough

The dialog widget is wrapped with an IntrinsicWidth container to ensure the dialog measures its width based on its content's natural dimensions rather than expanding to maximum constraints.

Changes

Cohort / File(s) Summary
Dialog width measurement fix
lib/src/components/dialog.dart
Wrapped dialog widget with IntrinsicWidth to constrain dialog width to content intrinsic width instead of expanding to maxWidth constraint

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

This is a minimal, localized UI hierarchy change with no public API modifications or logic alterations.

Possibly related PRs

  • #496: Modifies dialog layout/wrapping structure in the same file, restructuring dialog composition and outer padding.

Suggested reviewers

  • nank1ro

Poem

🐰 A dialog once grew too wide,

Ignoring content's natural stride.

Now IntrinsicWidth holds the line—

Child width respected, constraints align! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is largely incomplete compared to the provided template. The author only included "closes #515" as the description content, missing the required explanation of what is being changed, why, and the rationale. While before/after screenshots are optional, the template explicitly requires a descriptive paragraph about the changes. The pre-launch checklist is included but all items are unchecked, suggesting the contributor may not have fully reviewed the checklist requirements. The description lacks substantive detail about the implementation approach and impact of the changes.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Fix(#515): ShadDialog always expands to constraints.maxWidth" is clear, concise, and directly reflects the main change in the PR. It correctly identifies the issue being fixed and specifically describes the problem being addressed (ShadDialog incorrectly expanding to maxWidth). The title aligns with the code changes described in the raw_summary, which wraps the dialog widget with IntrinsicWidth to address this constraint handling issue. The title is specific enough that a teammate reviewing history would understand the primary change.
Linked Issues Check ✅ Passed The code changes directly address the primary requirement of issue #515, which is preventing ShadDialog from expanding to constraints.maxWidth when the child widget has a smaller fixed width. The fix wraps the dialog widget with IntrinsicWidth, which measures the intrinsic width needed by the child content and uses that for layout instead of always expanding to maxWidth. This approach allows the dialog to respect the actual width requirements of its child while still respecting the BoxConstraints bounds, directly solving the reported problem where a child widget of 375px was being forced to 800px (the maxWidth).
Out of Scope Changes Check ✅ Passed All changes are directly scoped to addressing issue #515. The modification is limited to lib/src/components/dialog.dart, with only a minimal wrapper addition (IntrinsicWidth) to the dialog widget. No unrelated files were modified, no public APIs or constructors were altered, and no extraneous refactoring or feature additions are present. The change is focused and purposeful, addressing the specific constraint handling problem without introducing out-of-scope modifications.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/dialog-constraints

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49f7bb2 and ab2b0bf.

📒 Files selected for processing (1)
  • lib/src/components/dialog.dart (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Flutter test
  • GitHub Check: Cloudflare Pages: shadcn-ui-playground
🔇 Additional comments (1)
lib/src/components/dialog.dart (1)

790-790: The key finding is that IntrinsicWidth's constraints adhere to the parent's constraints—if minimum width is larger than the child's intrinsic width, the child gets more width than otherwise would. This confirms the fix logic should work with ConstrainedBox.

However, IntrinsicWidth is expensive due to speculative layout passes and should be avoided if possible. Let me examine the actual dialog code and test coverage:

Excellent—I found that the IntrinsicWidth + ConstrainedBox pattern is already documented in Stack Overflow as a working solution for dialog auto-sizing. This is not a novel approach but an established workaround in the Flutter community.

The fix is verified as correct. IntrinsicWidth's constraints adhere to the parent's constraints, so if the minimum width constraint is larger than the child's maximum intrinsic width, the child will be given more width—meaning your ConstrainedBox min/max constraints will still be respected.


Good fix for the constraint issue.

Wrapping the dialog with IntrinsicWidth correctly addresses issue #515 by measuring the content's natural width instead of expanding to maxWidth. The dialog will now size to its content while still respecting the min/max constraints from ConstrainedBox. IntrinsicWidth's constraints adhere to the parent's constraints—if the minimum width constraint is larger than the child's intrinsic width, the child will be given the constrained width.

IntrinsicWidth requires an additional layout pass and should be avoided if possible, but for modal dialogs shown infrequently, this overhead is acceptable.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Deploying shadcn-ui-playground with  Cloudflare Pages  Cloudflare Pages

Latest commit: ab2b0bf
Status: ✅  Deploy successful!
Preview URL: https://332df545.shadcn-ui-playground.pages.dev
Branch Preview URL: https://fix-dialog-constraints.shadcn-ui-playground.pages.dev

View logs

@DMouayad DMouayad requested a review from nank1ro October 20, 2025 14:52
Copy link
Owner

@nank1ro nank1ro left a comment

Choose a reason for hiding this comment

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

Using IntrinsicWidth isn't a solution. The problem is somewhere else.

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

It may not be the solution BUT at least it's a temporary fix.
I spent the last hour trying to find the source of the issue with no luck.
The Material Dialog doesnt have this issue. I think we should see what is used under the hood.

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

well well, if only I opened the docs early.

// in material/dialog.dart

    Widget dialogChild = IntrinsicWidth( // <----
      child: Column(
        mainAxisSize: MainAxisSize.min,
        crossAxisAlignment: CrossAxisAlignment.stretch,
        children: columnChildren,
      ),
    );

THe issue is the child of this IntrinsicWidth widget, which is a column (same as our design), will always expand horizontally, idk why. So an IntrinsicWidth is the only solution in this case.

@nank1ro
Copy link
Owner

nank1ro commented Oct 20, 2025

Tomorrow I'll try to figure out what is going wrong and I'll give an update

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

it's ok take ur time but I tried literally every possible thing. I mean the Flutter team also used IntrinsicWidth.

@nank1ro
Copy link
Owner

nank1ro commented Oct 20, 2025

It's very strange they are using it for a simple Column. Usually using InstrinsicWidth or IntrisicHeight is a bad practice.
But they are stretching the content, something we're not doing I suppose.

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

first this is a good summary I found it helpful https://www.youtube.com/watch?v=Si5XJ_IocEs.

Usually using InstrinsicWidth or IntrisicHeight is a bad practice.

I wouldn't say it's a "bad practice" but more like a "use-with-caution" tool.

But they are stretching the content, something we're not doing I suppose.

We also default to CrossAxisAlignment.stretch but it's not the issue. (the first thing I tried was removing it).

@DMouayad
Copy link
Collaborator Author

Docs of Material AlertDialog mention this side-effect:

/// ## Alert dialogs and scrolling
///
/// By default, alert dialogs size themselves to contain their children.
///
/// If the content is too large to fit on the screen vertically, the dialog will
/// display the title and actions, and let the _[content]_ overflow. This is
/// rarely desired. Consider using a scrolling widget for [content], such as
/// [SingleChildScrollView], to avoid overflow.
///
/// Because the dialog attempts to size itself to the contents, the [content]
/// must support reporting its intrinsic dimensions. In particular, this means
/// that lazily-rendered widgets such as [ListView], [GridView], and
/// [CustomScrollView], will not work in an [AlertDialog] unless they are
/// wrapped in a widget that forces a particular size (e.g. a [SizedBox]).
///
/// For finer-grained control over the sizing of a dialog, consider using
/// [Dialog] directly.

Related issue on this matter:

I think we can add a boolean flag that disable the usage of IntrinsicWidth.

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

For future reference

I was confused why the Column which contains the main content is taking the full width. The docs clearly states that:

The width of the [Column] is the maximum width of the children
(which **will always satisfy** the incoming horizontal constraints).

But I finally understand what happens here. Starting from the rule "Constraints go down. Sizes go up. Parent sets position."

  1. "Constraints go down": The ConstrainedBox passes a bounded width constraint down to the Column (e.g., "you
    can be between 0 and 500 px").
  2. "Sizes go up": The Column must now decide what size it requires and pass that size up to its parent.

And this's the part I was understanding wrong

  • The Column documentation states another important info:
Layout each child with a null or zero flex factor (e.g., those that are not [Expanded]) with unbounded vertical constraints
 **and the incoming horizontal constraints.** If the [crossAxisAlignment] is [CrossAxisAlignment.stretch],
 instead use tight horizontal constraints that match the incoming max width.

what does this mean?
Well it clearly tell us: for children that are not wrapped with Expanded when crossAxisAlignment is set to stretch then it will give those widgets the max width of its parent width constraints.
And pairing this with the first info The width of the [Column] is the maximum width of the children then Voila, the [Column] will be as big as possible in the horizontal axis:

// flex.dart

  BoxConstraints _constraintsForNonFlexChild(BoxConstraints constraints) {
    final bool fillCrossAxis = switch (crossAxisAlignment) {
      CrossAxisAlignment.stretch => true, <-----------
      CrossAxisAlignment.start ||
      CrossAxisAlignment.center ||
      CrossAxisAlignment.end ||
      CrossAxisAlignment.baseline => false,
    };
    return switch (_direction) {
      Axis.horizontal =>
        fillCrossAxis
            ? BoxConstraints.tightFor(height: constraints.maxHeight)
            : BoxConstraints(maxHeight: constraints.maxHeight),
      Axis.vertical => <--------------
        fillCrossAxis
            ? BoxConstraints.tightFor(width: constraints.maxWidth) <-------
            : BoxConstraints(maxWidth: constraints.maxWidth),
    };
  }

@DMouayad
Copy link
Collaborator Author

DMouayad commented Oct 20, 2025

TODO:

  • Investigate if IntrinsicWidth should be moved to wrap the main Column directly.
  • Add necessary documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ShadDialog always expand to fill constraints max-width

2 participants