Skip to content

Conversation

@samliddleg
Copy link
Contributor

An optional lambda has been added to BpkModalBottomSheetCloseAction.Close. Overriding this does not disable onDismissRequest(). I.e, clicking the close button will invoke this optional lambda if it is not null, and then invoke onDismissRequest().

Remember to include the following changes:

  • Component README.md
  • Tests

If you are curious about how we review, please read through the code review guidelines

Copilot AI review requested due to automatic review settings December 17, 2025 10:14
@samliddleg Sam Liddle (samliddleg) added the patch A backwards compatible change/fix label Dec 17, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an optional onClick callback parameter to the close button of the bottom sheet modal component. The onClick callback executes before the dismiss request when users tap the close button, enabling additional actions like logging or state updates during the closing process.

Key Changes:

  • Added optional onClick lambda parameter to BpkModalBottomSheetCloseAction.Close
  • Integrated the callback invocation in the close button's click handler

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
BpkModalBottomSheet.kt Added optional onClick parameter to BpkModalBottomSheetCloseAction.Close data class
BpkModalBottomSheetImpl.kt Invokes the optional onClick callback when close button is clicked, before dismissing

Comment on lines +206 to 207
closeButton.onClick?.invoke()
onDismissRequest()
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The onClick callback is invoked after the hide animation completes via invokeOnCompletion. This may cause unexpected behavior if consumers expect their callback to execute before the sheet starts hiding. Consider invoking closeButton.onClick?.invoke() before coroutineScope.launch { state.hide() } to ensure the callback executes immediately when the button is clicked.

Copilot uses AI. Check for mistakes.
data object None : BpkModalBottomSheetCloseAction()
data class Close(
val contentDescription: String,
val onClick: (() -> Unit)? = null,
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The onClick parameter lacks documentation explaining when it is invoked relative to onDismissRequest and the sheet's hide animation. Add a KDoc comment explaining that this callback is invoked during the close button click handling and describing its relationship to the dismiss behavior.

Copilot uses AI. Check for mistakes.
data object None : BpkModalBottomSheetCloseAction()
data class Close(
val contentDescription: String,
val onClick: (() -> Unit)? = null,
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The new onClick callback parameter lacks test coverage. Add tests to verify that the callback is invoked when the close button is clicked, and that onDismissRequest is still called regardless of whether onClick is provided.

Copilot generated this review using guidance from repository custom instructions.
@skyscanner-backpack-bot
Copy link
Contributor

Warnings
⚠️ One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️

One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

⚠️

One or more component files were updated, but README.md wasn't updated. If your change contains API changes/additions or a new component please update the relevant component README.

Generated by 🚫 Danger Kotlin against 0062cf5

@skyscanner-backpack-bot
Copy link
Contributor

Warnings
⚠️ One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️

One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

Generated by 🚫 Danger Kotlin against 538b44b

@henrik-sky Henrik Sym (henrik-sky) added minor A new & backwards compatible feature/component and removed patch A backwards compatible change/fix labels Dec 18, 2025
@henrik-sky Henrik Sym (henrik-sky) merged commit b4d6059 into main Dec 19, 2025
10 checks passed
@henrik-sky Henrik Sym (henrik-sky) deleted the tempura/TEMPURA-3464 branch December 19, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor A new & backwards compatible feature/component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants