Skip to content

Remove back button when using end drawer - #63272

Merged
fluttergithubbot merged 6 commits into
flutter:masterfrom
gusghrlrl101:end_drawer
Sep 15, 2020
Merged

Remove back button when using end drawer#63272
fluttergithubbot merged 6 commits into
flutter:masterfrom
gusghrlrl101:end_drawer

Conversation

@gusghrlrl101

@gusghrlrl101 gusghrlrl101 commented Aug 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

flutter-dashboard Bot commented Aug 8, 2020

Copy link
Copy Markdown

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 p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Aug 8, 2020
@flutter-dashboard

flutter-dashboard Bot commented Aug 8, 2020

Copy link
Copy Markdown

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

darrenaustin commented Aug 27, 2020

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

gusghrlrl101 commented Aug 27, 2020

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

gusghrlrl101 commented Sep 9, 2020

Copy link
Copy Markdown
Contributor Author

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

darrenaustin
darrenaustin approved these changes Sep 10, 2020
View reviewed changes

@darrenaustin darrenaustin left a comment

Copy link
Copy Markdown
Contributor

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

gusghrlrl101 commented Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

@darrenaustin
Thank you for your feedback! 😊

@fluttergithubbot
fluttergithubbot merged commit dfd0c62 into flutter:master Sep 15, 2020
@pedromassango pedromassango mentioned this pull request Sep 24, 2020
Closed
@drddavi

drddavi commented Sep 30, 2020

Copy link
Copy Markdown

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

darrenaustin commented Oct 5, 2020

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

apgapg commented Oct 21, 2020

Copy link
Copy Markdown

@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

@chunhtai chunhtai mentioned this pull request Apr 13, 2021
Closed
@xu-baolin xu-baolin mentioned this pull request Apr 21, 2021
Closed
xu-baolin added a commit to xu-baolin/flutter that referenced this pull request Apr 22, 2021
@xu-baolin
Revert flutter#63272 and add more tests
e1c319a
@Hixie Hixie mentioned this pull request Feb 2, 2022
Open
@jja08111 jja08111 mentioned this pull request Feb 20, 2022
Closed
7 of 8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@darrenaustin darrenaustin darrenaustin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gusghrlrl101 @darrenaustin @drddavi @apgapg @xster @googlebot @fluttergithubbot