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

Disable maxWidth attribute in the bottom sheet #6067

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Apr 3, 2024

Closes #6066

Why is this the best possible solution? Were any other approaches considered?

By default the max width of the bottom sheet was 640px. I think there is no way to disable that attribute other than setting its value to big enough to make the sheet full-width.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Please just make sure that the bottom sheet is full-width.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review April 3, 2024 20:02
@grzesiek2010 grzesiek2010 requested a review from seadowg April 3, 2024 20:02
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

Are you able to file an issue at https://github.com/material-components/material-components-android about there not being a good way to remove the max width (as suggested in the Material Guidelines)? It doesn't feel like something we should have to hack around.

@@ -103,6 +103,7 @@
android:layout_height="match_parent"
android:background="?colorSurface"
app:behavior_hideable="true"
android:maxWidth="10000dp"
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this programmatically and use Integer.MAX_VALUE instead? It looks like that's what Android does internally.

I'd also be keen to make it more explicit what's going on here. We already have our own implementation of BottomSheetBehavior, so we could add a method like setMaxWidth(width: Int?) that just "disables" the max width on the underlying (actual) BottomSheetBehavior (by setting the max width to the MAX_VALUE).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it looks like android:maxWidth="@null" disables it correctly so we don't to set that attribute with any big number like 10000 or Integer.MAX_VALUE.
When it comes to doing that programmatically... I've tried it and it didn't work but either way, I think doing that in XML (the layout file) is better.

@seadowg seadowg merged commit ac772e8 into getodk:master Apr 8, 2024
6 checks passed
@dbemke
Copy link

dbemke commented Apr 9, 2024

I was wondering if it was possible to reduce the amount of empty space at the top of the bottom sheet? In the horizontal view when there are properties added to a point, the bottom sheet covers a half of the screen which makes looking at/using the map quite difficult. Even if there is the "errors/no errors" pill there is a bit of empty space at the top of the bottom sheet (covering the map). I know it's not a part of the PR exactly but just wanted to know what you think about the horizontal view.
verticalBottom

@grzesiek2010
Copy link
Member Author

I was wondering if it was possible to reduce the amount of empty space at the top of the bottom sheet? In the horizontal view when there are properties added to a point, the bottom sheet covers a half of the screen which makes looking at/using the map quite difficult. Even if there is the "errors/no errors" pill there is a bit of empty space at the top of the bottom sheet (covering the map). I know it's not a part of the PR exactly but just wanted to know what you think about the horizontal view.

So much margin is added by default see examples from https://github.com/material-components/material-components-android/blob/master/docs/components/BottomSheet.md
I don't know if we want to change it. What do you think @alyblenkin @seadowg?

@alyblenkin
Copy link
Collaborator

It's not a great user experience! It's worth considering changing the default. Google Maps has a nice adjustment that makes viewing and interacting easier. I think we could go ahead with the default for now and add this as a separate issue, which we can prioritize later. What do you think @grzesiek2010 @seadowg?

Screenshot 2024-04-09 at 9 36 07 AM

@grzesiek2010
Copy link
Member Author

Sounds good.

@dbemke
Copy link

dbemke commented Apr 11, 2024

Tested with Success!

Verified on devices with Androids: 10, 14

Verified cases:

  • issue The bottom sheet should be full-width #6066 is fixed
  • bottom sheet in the form map and select one from map forms
  • errors/ no errors pill in the bottom sheet
  • properties of a point in the bottom sheet
  • landscape and portrait view
  • dark and light mode

The top margin issue was filed separately

@WKobus
Copy link

WKobus commented Apr 11, 2024

Tested with Success!

Verified on device with Android 11

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.

The bottom sheet should be full-width
5 participants