Scalebar: Refactor ViewModel #715
Conversation
| } else { | ||
| mutableListOf() | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Why are we adding this else block?
The previous implementation was just doing this
if (style == ScalebarStyle.Bar || style == ScalebarStyle.Line) {
localLabels.lastOrNull()?.let {
_labels = mutableListOf(it)
}
} else {
_labels = localLabels
}There was a problem hiding this comment.
can we check early for default values of displayLength and lineMapLength and displayUnit and return an empty list rather than at the end here
| @@ -78,7 +78,7 @@ fun MainScreen(modifier: Modifier) { | |||
| .align(Alignment.BottomStart) | |||
| ) { | |||
| Scalebar( | |||
| maxWidth = 175.0, | |||
| maxDisplayWidth = 175.0, | |||
There was a problem hiding this comment.
this should just be maxWidth as the user is only concerned with what is shown on the display. From the user perspective there is only the width of the Scalebar being shown on the screen. I think this should simply be maxWidth
| @@ -55,7 +61,7 @@ import kotlin.time.Duration.Companion.seconds | |||
| */ | |||
| @Composable | |||
| public fun Scalebar( | |||
| maxWidth: Double, // maximum screen width allotted to the scalebar | |||
| maxDisplayWidth: Double, // maximum screen width allotted to the scalebar | |||
There was a problem hiding this comment.
this should just be maxWidth as the user is only concerned with what is shown on the display. From the user perspective there is only the width of the Scalebar being shown on the screen. I think this should simply be maxWidth
from the design,
The Scalebar must shrink and grow. It shouldn't grow past what the designer has said it's "width" is.
| @@ -201,15 +200,15 @@ private fun measureAvailableLineDisplayLength( | |||
| */ | |||
| @Composable | |||
| internal fun measureMinSegmentWidth( | |||
| lineMapLength: Double, | |||
| scalebarLineLength: Double, | |||
There was a problem hiding this comment.
We have 2 lengths for the scalebar. One is for the screen and its equivalent in map units(meters, feet etc). This is representing the second one. The first one we are calling displayLength. We can call this,
- scalebarLengthGeodetic,
- scalebarlengthInMapUnits,
- GeodeticLength,
- MapUnitLength
Related to issue: https://devtopia.esri.com/runtime/kotlin/issues/5245
Summary of changes:
NOTE: as agreed, we will come back to the other methods that need a composable context
Pre-merge Checklist