Skip to content

Conversation

@pogojotz
Copy link
Contributor

@pogojotz pogojotz commented May 24, 2025

This is modified and extended version of the merged and reverted PR #137415. It was reverted, because it broke a the sample app "Material 3 demo". This PR circumvents the breaking of existing code, by making the changes optional via constructor parameters. Default values for the parameters are functionally equivalent to the state before this change. The new key parameter for this is scrollable.

Additionally this change includes the parameters leadingAtTop: true and trailingAtBottom: false.

Fixes #89167

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 24, 2025
@pogojotz
Copy link
Contributor Author

pogojotz commented May 24, 2025

Reasoning for the additional parameters trailingAtBottom and leadingAtTop

There seems to be a use case for at least trailingAtBottom: true, as the mentioned "Material 3 demo" implements exactly that in a bit of a hacky approach. The sample app exploits the knowledge of the implementation detail, that trailing is part of the same Column as the navigation destination and using an Expanded widget to expand to bottom. There was an attempt to fix this in flutter/samples#2157, but that was dropped due to difficulties, which ultimately led to the revert (#143097) mentioned earlier.

I believe, by introducing more flexibility to the NavigationRail, less hacks will be implemented to achieve, what people would like to see in their apps. The Material guidelines do not mention anything about trailing widgets.

The leading widget seems to be address the "menu" and "FAB" buttons mentioned in the guidelines, which are always at the top. So I leave it up for debate, if leadingAtTop is actually required, but I think it might be good to have an additional degree of freedom.

@pogojotz pogojotz force-pushed the make-navrail-optionally-scrollable branch from 168bd28 to a42fbaf Compare May 25, 2025 15:50
@pogojotz pogojotz marked this pull request as ready for review May 25, 2025 17:07
@dkwingsmt dkwingsmt requested a review from QuncCccccc June 4, 2025 18:42
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM. The new properties make sense to me! Thank you for your contribution and patience!

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM, just some doc suggestion

this.useIndicator,
this.indicatorColor,
this.indicatorShape,
this.leadingAtTop = true,
Copy link
Member

Choose a reason for hiding this comment

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

just a thought: is it more clear to name them leadingPinnedAtTop and trailingPinnedAtBottom ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be fine with it. Just someone call it and I'll change it :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, I'm fine if this PR is merged as is!

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 27, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Jun 27, 2025
Merged via the queue into flutter:master with commit 5b6b621 Jun 27, 2025
71 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 27, 2025
@pogojotz pogojotz deleted the make-navrail-optionally-scrollable branch June 28, 2025 17:08
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
…169421)

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

This is modified and extended version of the merged and reverted PR
flutter#137415. It was reverted, because it broke a the sample app "Material 3
demo". This PR circumvents the breaking of existing code, by making the
changes optional via constructor parameters. Default values for the
parameters are functionally equivalent to the state before this change.
The new key parameter for this is `scrollable`.

Additionally this change includes the parameters `leadingAtTop: true`
and `trailingAtBottom: false`.

Fixes flutter#89167


## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
…169421)

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

This is modified and extended version of the merged and reverted PR
flutter#137415. It was reverted, because it broke a the sample app "Material 3
demo". This PR circumvents the breaking of existing code, by making the
changes optional via constructor parameters. Default values for the
parameters are functionally equivalent to the state before this change.
The new key parameter for this is `scrollable`.

Additionally this change includes the parameters `leadingAtTop: true`
and `trailingAtBottom: false`.

Fixes flutter#89167


## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Making NavigationRail scrollable

4 participants