-
Notifications
You must be signed in to change notification settings - Fork 122
[ios]Add a maximumWidthRatio
property to customize maximum width of scale bar.
#65
base: main
Are you sure you want to change the base?
Conversation
maximumWidthRatio
property to customize maximum width of scale bar.maximumWidthRatio
property to customize maximum width of scale bar.
update change log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Thank you for fixing this. I left some comments. Also please add tests to MGLMapViewScaleBarTests
I'm not sure this PR solves the problem in #52 e.g.
Does it? What do you think about adding an optional absolute maximum width, measured in points? Or are you suggesting that the developer modifies the ratio on rotation? |
Since we cannot know developer's UI requirements, I think developer should modify the ratio manually. @julianrex If developers display a map on a iPhone and support orientation, but one developer want a full screen width scale bar, but another want a half one. They both looks good in portrait mode, but when in landscape mode, if we make a limit absolute maximum width, it might be too short/long and out of expectations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I added two minor comments. And please fix the conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test (or add to the sample app) that checks the scenario of changing the ratio on device rotation. A view size change would also work here, so an integration test might be a good option here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@m-stephen - is this ready for re-review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know when I can get my back account and how I do it my maps open location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knov @m-stephen has left. Does this function need to continue? I can take over the job. /cc @julianrex |
@m-stephen @lloydsheng Long over due. Does the scalebar adjust accordingly when it's in a SplitView? |
@tsuz Per #52 (comment) and code, It should self-adjust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lloydsheng 👍 Thank you!
close: #52
This value is limited from 0.1 to 1.
Default value is 0.5