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

Simplified customizations on padding / itemPadding / corners for the main container / activeOpacity / shadows #82

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

viktar-d
Copy link

with that it is easy to change per project needs:

  • container padding (the bar itself)
  • corners for the bar
  • padding for each item
  • activeOpacity changed to activeBackgroundColor which defaults as it was to activeColor (blue) with 0.2 opacity
  • custom shadows for the bar and items as well

Pawel Petruch and others added 4 commits August 22, 2020 20:28
elevationShadow customization
padding and itemPadding customization
use activeBackgroundColor instead of opacity modifier
Copy link
Owner

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

This looks good, we just need to fix some nits.

Comment on lines 21 to 24
this.corner = const BorderRadius.only(
topLeft: Radius.circular(0),
topRight: Radius.circular(0),
),
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe just BorderRadius.zero here!

Copy link
Author

Choose a reason for hiding this comment

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

no issue, good catch

final EdgeInsets padding;

/// Defines padding for the item in a container, defaults to EdgeInsets.symmetric(vertical: 10, horizontal: 4)
final EdgeInsets itemPadding;
Copy link
Owner

Choose a reason for hiding this comment

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

Comments must end with a period!

Copy link
Author

Choose a reason for hiding this comment

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

that's easy, will do

Comment on lines -170 to +209
padding: EdgeInsets.symmetric(horizontal: 4),
padding: itemPadding,
Copy link
Owner

Choose a reason for hiding this comment

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

We need to make sure to not break any UI with those changes. So far, removing the horizontal padding may break some existing users

Copy link
Author

Choose a reason for hiding this comment

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

I'll change the defaults for itemPadding to be EdgeInsets.symmetric(horizontal: 4), that way it won't break anyone's apps

@pedromassango
Copy link
Owner

Please make sure to include some tests as well, to make sure those features does not break in the future.

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

Successfully merging this pull request may close these issues.

None yet

3 participants