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

refactor(Android): cleanup sheet behavior configuration #2420

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Oct 18, 2024

Description

I plant to extract most of the form sheet API related code out of ScreenStackFragment,
so that it is not polluted that much and more readable. Having experienced the #2045, where
big changes were rolled out at once, now I want to do it in couple of separate PRs.

This one simplifies sheet behaviour configuration logic in ScreenStackFragment by implementing
series of helper extension functions on BottomSheetBehavior.

Test code and steps to reproduce

Test1649

Checklist

Comment on lines +18 to +19
firstHeight: Int? = null,
secondHeight: Int? = null
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to stick with the original "peekHeight" and "maxHeight"?
Same applies to useSingleDetent and useThreeDetents.

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was to abstract the BottomSheetBehavior properties away as much as possible, because peekHeight is not the "first detent heightandmaxHeightis notsecondHeight` in general. They serve these functions only when other properties are set appropriately.

I've yet failed to abstract it away from useThreeDetents, because situation is a little bit more complicated there & requires another refactor, which will come in the future.

@kkafar kkafar requested a review from alduzy October 21, 2024 09:00
Copy link
Member

@alduzy alduzy left a comment

Choose a reason for hiding this comment

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

I am not 100% convinced with the names, but not against them either 😅 Great change overall, let's proceed

@kkafar kkafar merged commit 273194f into main Oct 21, 2024
4 checks passed
@kkafar kkafar deleted the @kkafar/refactor-sheet-detent-settings branch October 21, 2024 09:58
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