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

Add ability to customize ModalBottomSheet appearance in BottomSheetOverlay #1059

Merged

Conversation

chriswiesner
Copy link
Contributor

I've read that you're not accepting external contributions - raised the limitations of the BottomSheetOverlay in a discussion - but thought it might be a quick win if we could incorporate those changes directly.

This change adds the ability to customize the ModalBottomSheets appearance:

  • dragHandle
  • sheetShape
  • containerColor

Additionally it adds the ability to specify if the partiallyExpanded state of the sheet should be skipped. (There are use cases where the sheet should directly expand to it's full height)

Would be great if this could be incorporated.
Let me know if the optional parameters in the existing constructors are fine or there should be dedicated ones.

Copy link

Thanks for the contribution! Before we can merge this, we need @chriswiesner to sign the Salesforce Inc. Contributor License Agreement.

@chriswiesner
Copy link
Contributor Author

I figured that these customizations could also be done by creating a custom BottomSheetOverlay as all dependencies of it are public.
So, probably it is not necessary to incorporate these in the library 🤔

@ZacSweers
Copy link
Collaborator

I'm open to adding these, though I wonder if there's a way to make this more generic (maybe a @Composable () -> BottomSheetState param?). CC @stagg for thoughts

@stagg
Copy link
Collaborator

stagg commented Dec 19, 2023

I'm open to adding these, though I wonder if there's a way to make this more generic (maybe a @Composable () -> BottomSheetState param?). CC @stagg for thoughts

We could add a "DialogOverlay" where one could supply their own content and trigger the dismiss when appropriate. Mostly as a simple Overlay impl with prebuilt back handling and dismiss logic.

I'm cool with this for the bottom sheet though, not much more we can do to make it agnostic while still being a ModalBottomSheet.

@@ -42,35 +44,67 @@ private constructor(
private val model: Model,
private val dismissOnTapOutside: Boolean = true,
private val onDismiss: (() -> Result)? = null,
private val sheetShape: androidx.compose.ui.graphics.Shape? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets remove the full qualification of Shape here and on the public constructors.

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks!

@ZacSweers ZacSweers enabled auto-merge December 20, 2023 21:08
@ZacSweers ZacSweers added this pull request to the merge queue Dec 20, 2023
Merged via the queue into slackhq:main with commit d2b5325 Dec 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants