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.

Sign up for GitHub

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

Jump to bottom

Validate utility network #315

Open
wants to merge 9 commits into
base: v.next
Choose a base branch
from
Open

Validate utility network #315

wants to merge 9 commits into from
+1,248 −0

Conversation

shubham7109
Copy link
Collaborator

@shubham7109 shubham7109 commented Feb 3, 2025 •
edited
Loading

Description

PR to add a new Kotlin sample "Validate utility network" in Utility Networks category.

Links and Data

Sample issue: #3846

What To Review

  • Review the code to make sure it is easy to follow like other samples on Android
  • README.md and README.metadata.json files

How to Test

  • Run the sample on the sample viewer or the repo.

@shubham7109 shubham7109 added the New sample New Kotlin sample using ArcGIS Maps SDK label Feb 3, 2025
@shubham7109 shubham7109 self-assigned this Feb 3, 2025
@shubham7109 shubham7109 marked this pull request as ready for review February 3, 2025 22:52
TADraeseke
TADraeseke requested changes Feb 4, 2025
View reviewed changes
Copy link
Collaborator

@TADraeseke TADraeseke left a comment

Choose a reason for hiding this comment

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

LGTM! This is an excellent sample @shubham7109! Well done!

All suggestions are purely typo or ease of reading type suggestions.

samples/validate-utility-network-topology/README.md Outdated Show resolved Hide resolved
samples/validate-utility-network-topology/README.md Outdated Show resolved Hide resolved
samples/validate-utility-network-topology/README.md Outdated Show resolved Hide resolved
samples/validate-utility-network-topology/README.md Outdated Show resolved Hide resolved
samples/validate-utility-network-topology/README.md Outdated Show resolved Hide resolved
.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Show resolved Hide resolved
.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Outdated Show resolved Hide resolved
.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Outdated Show resolved Hide resolved
.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Outdated Show resolved Hide resolved
.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Outdated Show resolved Hide resolved
@shubham7109
Copy link
Collaborator Author

@TADraeseke Thank you for the kind words and the helpful PR feedback. I added the changes, and is now ready for your review!

TADraeseke
TADraeseke approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@TADraeseke TADraeseke left a comment

Choose a reason for hiding this comment

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

Brilliant sample!

@eri9000 eri9000 self-requested a review February 5, 2025 21:33
eri9000
eri9000 reviewed Feb 5, 2025
View reviewed changes
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.

@shubham7109
Good work,
some suggestions. Let me know if you'd like to discuss further

.../sample/validateutilitynetworktopology/components/ValidateUtilityNetworkTopologyViewModel.kt Show resolved Hide resolved
...ismaps/sample/validateutilitynetworktopology/screens/ValidateUtilityNetworkTopologyScreen.kt
Comment on lines +87 to +92
LaunchedEffect(Unit) {
if (!isViewmodelInitialized) {
mapViewModel.initialize()
isViewmodelInitialized = true
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be handled in the viewmodel's init block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leaving this as is, if that's alright!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isViewmodelInitialized is currently rememberSaveable which is needed, if not initialize() will be triggered on every rotation causing an object already owned error.

...ismaps/sample/validateutilitynetworktopology/screens/ValidateUtilityNetworkTopologyScreen.kt Outdated Show resolved Hide resolved
@shubham7109
Copy link
Collaborator Author

@eri9000 Thanks for the feedback. Ready for your review.

@shubham7109 shubham7109 requested a review from eri9000 February 5, 2025 23:57
eri9000
eri9000 approved these changes Feb 6, 2025
View reviewed changes
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.

LGTM 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eri9000 eri9000 eri9000 approved these changes

@TADraeseke TADraeseke TADraeseke approved these changes

Assignees

@shubham7109 shubham7109

Labels
New sample New Kotlin sample using ArcGIS Maps SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shubham7109 @TADraeseke @eri9000