packages
packages copied to clipboard
[flutter_adaptive_scaffold] Make drawer items scrollable
This PR makes the drawer navigation list scrollable. Previously, if a large amount of navigation items were used, they would be cut off.
Fixes flutter/flutter#114175
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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I listed at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style. - [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] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).
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.
@gspencergoog From triage: Ping on this review?
Hi @percula, sorry, it seems that our continuous integration system has updated a token since you synced to the head revision.
Can you sync to the head revision on the main branch and rebase your changes, please? That should fix the remaining issues.
Looks like you might have merged instead of rebased. You'll want to rebase your changes and resolve the conflicts.
Let me know if you need some help with that.
@gspencergoog Sorry about that, I'm pretty sure I selected "rebase" in Android Studio but it did do something funky after that 🤷. Should be good now!
Oh, I see that it doesn't actually have any tests for the new scrollable functionality. Could you add a test for that so that it doesn't get broken in the future?
What's the status of this PR? Is it still waiting for more tests?
Yeah, sorry I've been a bit busy and haven't had time to write a test. Hopefully after the holidays I can get to it.
@percula Checking in from our regular triage meeting; do you think you'll have time to add tests to this PR soon?
@stuartmorgan @a-wallen Thanks for your patience! I've just added a commit with a test. However, I have to admit that even after 3.5 years of Flutter development, this is the very first test I've tried to write. And I wasn't successful. I'm having difficulty getting the test to find and tap the hamburger menu icon to show the drawer. Any help would be greatly appreciated 🙂.
@percula I can help you, but we have another issue: Alex has left the team, and since he's requested changes (and probably won't be checking back later to approve), we won't be able to land this particular PR because GitHub's UI doesn't have a way to override that.
If you could open a new PR with the same contents, we can proceed with figuring out your tests.
@a-wallen Thanks Alex, I appreciate it!
OK, @percula ,so now we just need to get this rebased onto the latest HEAD to get rid of the conflicts. And then we can deal with the test failure.
@percula Are you still planning on updating this to the latest main per the discussion above?
Yep! Sorry, just got lost in my inbox. I'll do this tonight
@gspencergoog Ok, my fork is rebased on the latest main. But the test is failing, can you help figure out why? As I mentioned, I don't have much experience with test and was having difficulty getting the test to find the hamburger menu and click it, so that the test could verify it can scroll to the last destination item.
Looks like there are at least two things:
- The test packages/flutter_adaptive_scaffold/test/adaptive_scaffold_test.dart is failing. Try running that locally to debug it. It looks like it can't find the widget to tap on that it used to find before your change.
- There's a type annotation missing on packages/flutter_adaptive_scaffold/test/adaptive_scaffold_test.dart:248:62: "Missing type annotation. Try specifying the type 'NavigationDestination'."
2 is an easy fix. Just need to commit the change.
1 though...I tried for over an hour on my local machine and got nowhere. I was hoping someone on the Flutter team might know of an existing test that taps the hamburger button, so I can copy that behavior and fix my test. Any ideas?
@percula Should this be marked as a draft? Have you reached out about your test issues on Discord?
Marking as a draft due to lack of updates; please feel free to mark as ready for review once the above is addressed.
@percula Thanks for your contribution. It sounds like you don't have the bandwidth to work on this right now, so I'm going to close this PR and let people know in the issue that you've done the bulk of the work and if anyone wants to finish it that they can start with your PR here. Thanks again for what you've done so far, and if you ever want to try to resume this work please don't hesitate to reach out on our discord (link in the contributing guidelines, see below)!
@Hixie sounds good. I'm still open to working on this, but I got stuck when writing the test. I don't know how to get the test to tap the hamburger menu. If anyone knows how to do that, I'm all ears 🙂