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

[android] fix the overlapping of bottom sheet and map buttons #7696

Merged

Conversation

kavikhalique
Copy link
Contributor

fix #7085

1 - Increased height of map buttons by changing nav_menu_height value from 70sp to 80sp, in order to create more space for bottom sheet nav.
2 - Increased and fixed the size of text containing layout in order to avoid fluctuations in size of bottom sheet during navigation due to change in text.

fix1

Signed-off-by: kavikhalique <kavikhalique3@gmail.com>
Copy link
Member

@Jean-BaptisteC Jean-BaptisteC left a comment

Choose a reason for hiding this comment

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

✅ Pixel 6 - Android 14

@biodranik
Copy link
Member

Did you test it with increased/the largest font sizes in the system?

@kavikhalique
Copy link
Contributor Author

Did you test it with increased/the largest font sizes in the system?

yes i tested and it works fine.

There is no overlapping of bottom sheet and upper buttons.
Although there is overlapping of direction symbol and buttons but i guess thats separate issue.

check2

check1

@@ -112,7 +112,7 @@
<dimen name="nav_button_height">@dimen/primary_button_min_height</dimen>
<dimen name="nav_icon_size">48dp</dimen>
<dimen name="nav_bottom_gap">12dp</dimen>
<dimen name="nav_menu_height">70sp</dimen>
<dimen name="nav_menu_height">80sp</dimen>
Copy link
Member

Choose a reason for hiding this comment

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

Nav panel now takes more map space, is it a really necessary change? Are there ways to fix issues without increasing the size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since arabic characters take some extra spaces than normal english so we need to increase the size of nav in any case.

The other option could be to decrease the size of text little bit inside the bottom sheet nav then also it can get solved, but text might feel difficult to read for some people.

Copy link
Member

@biodranik biodranik Mar 27, 2024

Choose a reason for hiding this comment

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

Increasing the size manually will work until on some devices users set an even larger font that won't fit into the increased size. Right?

Android has wrap_content property that scales for the text size automatically (there are also other ways to scale). Why automatic scaling doesn't work here for Arabic, but works for other fonts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Please take this also into consideration that the height of buttons(above bottom sheet) have hardcoded height of 70sp (from before) so even if wrap content will be variable it will be of no use because it will overlap on buttons in case of very bigger texts.(and this was the actual problem of overlapping)

  • The size is defined in "sp" unit, its kind of variable with the font size of system so it will change the overall size to make characters fit into it, if the user increases there font size.

Although i can revert back to wrap content as well and it will not create any problem in general, but in some cases bottom sheet size will fluctuate during navigation with change of text into it since its of variable size.

@@ -2,7 +2,7 @@
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_height="65sp"
Copy link
Member

Choose a reason for hiding this comment

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

Why wrap_content didn't work, but hard-coding a constant works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually overlapping problem was not occurring for every arabic characters. That means bottom nav height was changing with the change in characters inside bottom sheet.

Since for buttons it was already hardcoded height so keeping bottom sheet nav height variable makes no sense and this was the reason of overlapping as well because when bottom sheet size was increasing it was getting overlapped.

I hardcoded the bottom sheet to avoid fluctuations of bottom sheet nav's height with the change in text inside it.

Since nav bar height for buttons was hardcoded so i did the same for bottom sheet as well.

Copy link
Member

Choose a reason for hiding this comment

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

Hard-coding may not work well if the system font size is very large, right?

What are the ways to implement it without hard-coding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since buttons above the bottom sheet are hardcoded where as bottom sheet below it have variable height which is the root cause of problem.

We can do two things to avoid this overlapping

  1. Move these two buttons to the right side to avoid overlapping.

I checked on google maps they also have similar kind of bottom sheet navigation layout. To avoid overlapping due to less space they keep all the buttons on right side.

image

This also allows users to use those buttons even in expanded mode

  1. We can also make height of buttons dynamic dependent on bottom sheet height so that it always keep a defined margin from the bottom sheet view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the ways to implement it without hard-coding?

Will it be okay if i just increase the button height little bit and keep the bottom sheet as wrap content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@biodranik sir please review it once.
According to me there are two options available. Either restructure the landscape navigation ui to make it future proof or solve this problem temporarily until any issue comes

My above solution basically solves around 95% of problem but still if someone enlarges the text size fully then everything dont fits in screen.

Please suggest me what should be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, changing wrap_content to 65sp seems controversial. However, it does solve the issue with Arabic and other characters. The landscape navigation UI could be restructured, but it would take significantly more time and effort to implement and test. Let's be rational here and merge the proposed workaround until a better solution is found.

@biodranik
Copy link
Member

On both screenshots the next turn rectangle can be moved up, and the top indent in the bottom nav info panel can be reduced. Will it help to solve the issue with increased font?

@kavikhalique
Copy link
Contributor Author

On both screenshots the next turn rectangle can be moved up, and the top indent in the bottom nav info panel can be reduced. Will it help to solve the issue with increased font?

Yes sir it can be done but the problem is that all components of the UI are working independent of each others means they don't adjust there position according to other components.

Buttons above bottom nav are in different layout and there height is hardcoded according to an assumed height of bottom nav

Similarly other components heights and positions are also hard coded which is arising problem since it is not dynamic its facing overlap issues

If we move the turn symbol little up it will be up in all the cases either its small font or large so it may not look good in general case.

Please guide me what should be the best option.

@rtsisyk
Copy link
Contributor

rtsisyk commented Apr 26, 2024

✅ Samsung S10 - Android 12

@rtsisyk rtsisyk merged commit c986908 into organicmaps:master Apr 26, 2024
6 checks passed
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.

Increase space between bottomsheet and buttons in navigation
4 participants