packages icon indicating copy to clipboard operation
packages copied to clipboard

[flutter_adaptive_scaffold] Make drawer items scrollable

Open percula opened this issue 1 year ago • 18 comments

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.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to 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.

percula avatar Oct 27 '22 19:10 percula

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.

flutter-dashboard[bot] avatar Oct 27 '22 19:10 flutter-dashboard[bot]

@gspencergoog From triage: Ping on this review?

stuartmorgan avatar Nov 15 '22 21:11 stuartmorgan

@percula Looks like the formatting needs to be fixed.

See the error for a command that will fix it.

gspencergoog avatar Nov 17 '22 02:11 gspencergoog

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.

gspencergoog avatar Nov 17 '22 18:11 gspencergoog

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 avatar Nov 17 '22 20:11 gspencergoog

@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!

percula avatar Nov 18 '22 02:11 percula

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?

gspencergoog avatar Nov 18 '22 05:11 gspencergoog

What's the status of this PR? Is it still waiting for more tests?

stuartmorgan avatar Dec 13 '22 21:12 stuartmorgan

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 avatar Dec 22 '22 18:12 percula

@percula Checking in from our regular triage meeting; do you think you'll have time to add tests to this PR soon?

stuartmorgan avatar Jan 31 '23 20:01 stuartmorgan

@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 avatar Feb 03 '23 02:02 percula

@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.

gspencergoog avatar Feb 10 '23 17:02 gspencergoog

@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.

gspencergoog avatar Feb 10 '23 18:02 gspencergoog

@percula Are you still planning on updating this to the latest main per the discussion above?

stuartmorgan avatar Feb 28 '23 20:02 stuartmorgan

Yep! Sorry, just got lost in my inbox. I'll do this tonight

percula avatar Feb 28 '23 23:02 percula

@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.

percula avatar Mar 06 '23 19:03 percula

Looks like there are at least two things:

  1. 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.
  2. 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'."

gspencergoog avatar Mar 06 '23 20:03 gspencergoog

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 avatar Mar 07 '23 02:03 percula

@percula Should this be marked as a draft? Have you reached out about your test issues on Discord?

stuartmorgan avatar Mar 28 '23 19:03 stuartmorgan

Marking as a draft due to lack of updates; please feel free to mark as ready for review once the above is addressed.

stuartmorgan avatar Apr 24 '23 18:04 stuartmorgan

@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 avatar Jun 13 '23 22:06 Hixie

@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 🙂

percula avatar Jun 13 '23 23:06 percula