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

[New Layout] Adds peek height (min height) to new layout bottom sheets #7103

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Sep 12, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Adds peek height to new layout bottom sheets so they don't show at heights smaller than they should be

Motivation and context

Fixes #7027

Screenshots / GIFs

Tests

  • Turn phone on landscape
  • Open space list sheet and new chat sheets. See that they don't show at a height that's too small

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

@ericdecanini ericdecanini added the PR-Small PR with less than 20 updated lines label Sep 12, 2022
@ericdecanini ericdecanini marked this pull request as ready for review September 12, 2022 21:49
import androidx.annotation.FloatRange
import com.google.android.material.bottomsheet.BottomSheetDialog

@Suppress("DEPRECATION")
Copy link
Contributor

Choose a reason for hiding this comment

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

WindowManager. defaultDisplay and getMetrics are deprecated in 30. Could you add API 30 implementation? And also, please, use suppress on a line, where it's needed, not on whole function

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Some non blocking remarks, but that would deserved to be handled at some point.

} else {
setPeekHeightPreApi30(percentage)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I would rather create an extension getScreenheightPx() to get the Window height, which will be more reusable.
Then just call

val height = window?.getScreenheightPx()
behavior.setPeekHeight((height * percentage).toInt(), true)

here.

We have some equivalent code at this point: https://github.com/vector-im/element-android/blob/main/vector/src/main/java/im/vector/app/features/roomprofile/uploads/media/RoomUploadsMediaFragment.kt#L86

It would deserve a quick refactor to extract useful and reusable and compatible extensions, in the package im.vector.app.core.extensions. Happy to do it later if you do not have time.

return super.onCreateDialog(savedInstanceState).apply {
(this as BottomSheetDialog).setPeekHeightAsScreenPercentage(0.5f)
}
}
Copy link
Member

@bmarty bmarty Sep 14, 2022

Choose a reason for hiding this comment

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

All those new BottomSheet should extend VectorBaseBottomSheetDialogFragment, for analytics support and common behavior. This specific treatment could then be moved to the parent, to avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to make the changes 👍

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ericdecanini ericdecanini merged commit 01e1d74 into develop Sep 15, 2022
@ericdecanini ericdecanini deleted the bugfix/eric/landscape-bottom-sheet-peek branch September 15, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Layout: Bottom sheet padding missing (and landscape mode issue)
3 participants