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

Fix issues #1

Merged
merged 1 commit into from
Feb 1, 2020
Merged

Fix issues #1

merged 1 commit into from
Feb 1, 2020

Conversation

ricknout
Copy link
Contributor

Hi @Sottti,

I have addressed most of the issues you were asking about in this PR:

  • Changed to using a FrameLayout instead of a MaterialCardView for the root bottom sheet view. It is not necessary to use a card here as BottomSheetBehavior applies a MaterialShapeDrawable background to the root view
  • Adjusted the styles - notice for bottom sheets you use backgroundTint for the bg color
  • Set some default styles in the app theme and used color/style attrs where appropriate

To answer your other question - yes the animating of the bottom sheet corners to 0dp at the top is intentional. Not everyone is too happy about it though - see this issue and this pull request.

@Sottti Sottti merged commit 93b3325 into Sottti:master Feb 1, 2020
@Sottti
Copy link
Owner

Sottti commented Feb 1, 2020

@ricknout thanks for that. Starts to make sense now.

I have some questions:

  • How is ShapeAppearance.MediumComponent relevant on this sample project? I've seen you've added it but now sure how is changing things.

  • Looks to me you need to be explicit on the bottomSheetStyle. I mean you need to add <item name="bottomSheetStyle">@style/Widget.MaterialComponents.BottomSheet</item> for it to take the colorSurface as the bottom sheet backgroundTint. How comes this is not the default behaviour when extending Theme.MaterialComponents?

  • How comes the com.google.android.material.bottomsheet.BottomSheetBehavior does a lot of wizery but it doesn't apply the style="?attr/bottomSheetStyle" automatically? I mean if you apply the behaviour but not the style, then it doesn't default to the bottomSheetStyle

  • I find there is some important information missing here, at least regarding styling and theming. Is this because is still not stable?

Thanks!

@ricknout
Copy link
Contributor Author

ricknout commented Feb 3, 2020

  • This is basically theming the shape of all components in your app which are considered "medium" size (which includes cards). You could just theme the card shape, as you had done before, by using shapeAppearance/shapeAppearanceOverlay in the card style. But I would recommend doing it at an app level for consistency.

  • colorSurface actually is the default backgroundTint. But yes, you need to specify style="?attr/bottomSheetStyle in the layout

  • Good question. I believe it's because of this. So it checks if the shapeThemingEnabled attr is present but this attr lives inside the bottom sheet style itself. Maybe it could be considered a bug, but sometimes different backgrounds are desired for bottom sheets.

  • We are working on improving the docs for MDC-Android in general. You can see a sneak peek of the new format for buttons here. The same will eventually be done for bottom sheets.

@Sottti
Copy link
Owner

Sottti commented Feb 4, 2020

@ricknout thanks a lot!

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.

2 participants