Skip to content

Conversation

@hyochan
Copy link

@hyochan hyochan commented Oct 4, 2018

Previously, the top of the touch area of BottomNavigationBarItem is slightly cut off.

Previous result:
previous

Fixed result:
fixed

Previously, the top of touch area of BottomNavigationBarItem is slightly cutted off.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@hyochan
Copy link
Author

hyochan commented Oct 4, 2018

I've submitted cla just now. Thanks.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 4, 2018
Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @dooboolab!

Could you add a test in bottom_navigation_bar_test.dart that would have caught this problem? It'll ensure we don't accidentally regress it in the future.

@tvolkert
Copy link
Contributor

/cc @HansMuller

Since, giving additionalBottomPadding with symmetric, we need to multiply the value by 2.
@hyochan
Copy link
Author

hyochan commented Oct 24, 2018

@tvolkert I've like to write up test for this too. However, how can I test if my test code is working correctly? I could fix this problem in Android Studio manually editing flutter's code. However, I currently have no idea how to test the flutter itself.

Could you provide me some guideline?

@HansMuller
Copy link
Contributor

Thanks for your contribution. The before and after animated GIFs are superb!

Unfortunately, we need to address two problems before landing this fix:

  • It would be best if we did not change the height of the BottomNavigationBar. Doing so could disturb existing application layouts and tests.
  • The custom circle ink splash (_Circle) in bottom_navigator.dart no longer matches the Material spec. The tap feedback is now supposed to be an ink ripple.

This is no longer a simple change however if you'd like to attempt it, I can help.

@HansMuller
Copy link
Contributor

The ink ripple effect that BottomNavigationBar is supposed to provide looks like this:

bottom_navigation_bar_ripple

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 28, 2018
@hyochan
Copy link
Author

hyochan commented Jan 16, 2019

I'll try to manage above when #22956 is merged because current PR affect these codes.

@hyochan
Copy link
Author

hyochan commented Feb 19, 2019

I was going to continue working on this but it seems that this has been taken in #28159.

@hyochan hyochan closed this Feb 19, 2019
@ryan-sf
Copy link

ryan-sf commented Jan 17, 2020

I was going to continue working on this but it seems that this has been taken in #28159.

The ripple effect still does not match the Material spec when I use it. Are there any examples of this working in the wild?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

6 participants