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

Add Changes for DualUnitLine #736

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

puneet-pdx
Copy link
Collaborator

@puneet-pdx puneet-pdx commented Feb 3, 2025

Related to issue: # https://devtopia.esri.com/runtime/kotlin/issues/5194

UI and logic to display Dual unit line scalebar

Summary of changes:

  • Add composable to draw Dual unit line scalebar
  • Add preview
  • Add fun to determine the alternate unit display length and label text
  • Add test

Pre-merge Checklist

@puneet-pdx puneet-pdx self-assigned this Feb 3, 2025
@puneet-pdx puneet-pdx requested a review from eri9000 February 3, 2025 19:00
Copy link
Collaborator

@eri9000 eri9000 left a comment

Choose a reason for hiding this comment

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

@puneet-pdx Looks good overall, some small suggestions

Comment on lines 521 to 522
val endScalebarDivision = ScalebarDivision(displayLength,"3000 m")
val alternateScalebarDivision = ScalebarDivision(0.75 * displayLength,"3500 Km")
Copy link
Collaborator

Choose a reason for hiding this comment

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

these values should be flipped sine the primary label is always bigger than the secondary label

Suggested change
val endScalebarDivision = ScalebarDivision(displayLength,"3000 m")
val alternateScalebarDivision = ScalebarDivision(0.75 * displayLength,"3500 Km")
val endScalebarDivision = ScalebarDivision(displayLength,"3500 mi")
val alternateScalebarDivision = ScalebarDivision(0.75 * displayLength,"3000 Km")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, not necessarily
Screenshot_20250203_125328

* @since 200.7.0
*/
@Composable
internal fun DualUnitLineScalebar(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that this does not work as expected when we apply padding to the modifier in the Scalebar composable and also does not work when the alignment is applied on Scalebar instead of the box.
I say we merge as is right now and I can fix it as part of my other PR

Copy link
Collaborator

@eri9000 eri9000 Feb 3, 2025

Choose a reason for hiding this comment

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

this will also need displayWidth to be passed in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am assuming you mean displayLength and
No, primaryScalebarDivision provides the length of the horizontal line drawn

@puneet-pdx
Copy link
Collaborator Author

@eri9000 thanks. Merging

@puneet-pdx puneet-pdx merged commit c8a6358 into feature-branches/scalebar Feb 3, 2025
@puneet-pdx puneet-pdx deleted the puneet/5194_DualUnitLIne_2 branch February 3, 2025 20:57
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.

2 participants