Add header and footer support to NavigationDrawer #168005
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
QuncCccccc
left a comment
There was a problem hiding this comment.
Seems there are no tests yet. Could you help add tests for the PR change:)?
|
Hi @QuncCccccc, per requested, I have included a test for testing the presence of the header/footer if provided. Please let me know if I missed anything or any further changes. 🙏 |
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM. Thank you for your contribution:)
|
Seems a format issue breaks Linux analyze: |
|
Hi @QuncCccccc I don't quite understand what's causing this fail, could you help take a look? |
|
I think it might be an infra error. Can we try rebase master to retrigger the test again:)? |
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍. Thanks for the PR!
I've been seeing a lot of similar Google test failures recently, where it just says "Deleted". See #169159. If a rebase doesn't help here then we can probably land this without Google tests as it looks low-risk to me.
| bottom: false, | ||
| child: Column( | ||
| children: <Widget>[ | ||
| if (header != null) header!, |
There was a problem hiding this comment.
Do we have support for null aware elements in Flutter yet? If so I think this could be ?header.
There was a problem hiding this comment.
That would require bumping the sdk version to 3.8 in pubspec.yaml (currently 3.7)?
Wouldn't the changes would be too big for such a small PR when applying the dart format <...>?
There was a problem hiding this comment.
That would require bumping the sdk version to 3.8 in
pubspec.yaml(currently 3.7)? Wouldn't the changes would be too big for such a small PR when applying thedart format <...>?
I've done that, doesnt work yet. Was it supposed to work?
There was a problem hiding this comment.
That would require bumping the sdk version to 3.8 in
pubspec.yaml(currently 3.7)? Wouldn't the changes would be too big for such a small PR when applying thedart format <...>?I've done that, doesnt work yet. Was it supposed to work?
Until now, I don't find increments of the sdk version in pubspec.yaml in master branch.
I tried modification below on local machine:
# flutter/packages/flutter/pubspec.yaml
environment:
- sdk: ^3.7.0-0
+ sdk: ^3.8.0-0The linter recognizes the null-aware expression.
I just think this should be in another PR?
There was a problem hiding this comment.
Yeah, changing it in a separate PR sounds good to me!
b194866 to
3c4c69e
Compare
3c4c69e to
f525491
Compare
<!-- 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 --> ## Description This PR attempts to add header and footer for `NavigationDrawer` widget as shown below:  ## Issues are fixed by this PR - flutter#127621 - flutter#135750 (partially) ## 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
Description
This PR attempts to add header and footer for
NavigationDrawerwidget as shown below:Issues are fixed by this PR
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.