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

Remove back button when using end drawer #63272

Merged
merged 6 commits into from Sep 15, 2020

Conversation

gusghrlrl101
Copy link
Contributor

@gusghrlrl101 gusghrlrl101 commented Aug 8, 2020

Description

Before

before

When end drawer opens, close button is showing on leading.

After

after

When end drawer opens, close button is not shown.

Related Issues

#13601

Tests

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

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

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 8, 2020
@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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@darrenaustin
Copy link
Contributor

@gusghrlrl101 thanks for the contribution. Sorry for the delay in review.

To be honest, I am not sure exactly what you are trying to solve with this. What is wrong with the back button showing when you are using a drawer from the right side? I looked at the spec, but I didn't see any guidance on this (other than to not use end drawers for navigation 😄 ). Even if the user sees the back arrow it in the darkened scrim on the left, if they tap on it, it will dismiss the drawer anyway, so I don't see the harm in having it there.

Is there a use case for this that I am missing?

All that said, if we decide to do this, your fix looks good. However, if we do proceed, we will need to have a test added that covers this change (see test/material/app_bar_test.dart).

@gusghrlrl101
Copy link
Contributor Author

@darrenaustin, Thanks for your reply 😊.

I added test code for AppBar's leading is null when endDrawer exists.

I think it is unnecessary to show the button and it is awkward for user.
As you said, if we touch darkened scrim on the left, endDrawer will disappear. If so, since we can't even press the back button, isn't it more unnecessary?
Opening endDrawer is the meaning of showing the menu rather than the meaning of navigating. So it is awkward from the user's point of view when the back button appears after opening the menu.
Seeing that people have tried to get rid of the button in this issue as well, I think many users feel it is awkward too.
I would appreciate it if you give me any comments.

Thank you 😊.

@gusghrlrl101
Copy link
Contributor Author

@darrenaustin
I'm sorry to menthion again.
Could you check out my PR again?
Thanks 😊.

Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

Sorry for the lag on this.

I am still not entirely convinced this is that necessary, but it does look like a reasonable implementation and people have been reporting it, so let's go with it.

LGTM. I will push this after they cut a new branch for release (which should be happening soon).

Thanks for the contribution!

@gusghrlrl101
Copy link
Contributor Author

@darrenaustin
Thank you for your feedback! 😊

@drdDavi
Copy link

drdDavi commented Sep 30, 2020

Would have been great if there was a boolean to disable and enable this new feature, this new behavior broke my app... this new change should not be the default.
Adding the code below to fix several pages...
leading: IconButton(icon: Icon(Icons.close), onPressed: () => Get.back()),

@darrenaustin
Copy link
Contributor

@drdDavi so sorry this broke your app. Can you give us more information on how exactly it did that? How were users even tapping the icon with the drawer open?

@apgapg
Copy link

apgapg commented Oct 21, 2020

@darrenaustin Before this back button and end drawer button both appeared. But now with 1.22.x, only end drawer button is shown.
This happens only when one sets endDrawer. In rest pages back button is visible

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.

None yet

7 participants