Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flutter_adaptive_scaffold] Make drawer items scrollable #2744

Closed
wants to merge 6 commits into from

Conversation

percula
Copy link

@percula percula commented Oct 27, 2022

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard
Copy link

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.

@stuartmorgan
Copy link
Contributor

@gspencergoog From triage: Ping on this review?

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Sorry for the slow review. cc @a-wallen for second review.

@gspencergoog
Copy link
Contributor

@percula Looks like the formatting needs to be fixed.

See the error for a command that will fix it.

@gspencergoog
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@a-wallen a-wallen left a comment

Choose a reason for hiding this comment

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

@gspencergoog the test LGTM. I'll approve so that this one can get merged.

@gspencergoog
Copy link
Contributor

gspencergoog commented Feb 10, 2023

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

@stuartmorgan
Copy link
Contributor

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

@percula
Copy link
Author

percula commented Feb 28, 2023

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

# Conflicts:
#	packages/flutter_adaptive_scaffold/CHANGELOG.md
#	packages/flutter_adaptive_scaffold/lib/src/adaptive_scaffold.dart
#	packages/flutter_adaptive_scaffold/pubspec.yaml
#	packages/flutter_adaptive_scaffold/test/adaptive_scaffold_test.dart
@percula
Copy link
Author

percula commented Mar 6, 2023

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

@gspencergoog
Copy link
Contributor

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

@percula
Copy link
Author

percula commented Mar 7, 2023

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?

@stuartmorgan
Copy link
Contributor

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

@stuartmorgan
Copy link
Contributor

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

@stuartmorgan stuartmorgan marked this pull request as draft April 24, 2023 18:16
@Hixie
Copy link
Contributor

Hixie commented Jun 13, 2023

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

@percula
Copy link
Author

percula commented Jun 13, 2023

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

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

Successfully merging this pull request may close these issues.

[flutter_adaptive_scaffold] Drawer items do not scroll
5 participants