Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

New: Create load report #1043

Merged
merged 26 commits into from
Mar 29, 2021
Merged

New: Create load report #1043

merged 26 commits into from
Mar 29, 2021

Conversation

yo1995
Copy link
Collaborator

@yo1995 yo1995 commented Mar 5, 2021 •
edited
Loading

This PR adds the new Create load report sample in Utility network category. The issue on DT: common-samples/issues/2594

Link to the branch: here. Apologies for the wrong branch name - it was too late when I realized that I wasn't on the right branch 😅

@yo1995 yo1995 self-assigned this Mar 5, 2021
@yo1995 yo1995 requested a review from vquach2404 March 5, 2021 23:20
vquach2404
vquach2404 reviewed Mar 6, 2021
View reviewed changes
Copy link
Collaborator

@vquach2404 vquach2404 left a comment

Choose a reason for hiding this comment

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

A few points to discuss:

  • Is it possible to individually delete some of the phases? Maybe it can be similar to Configure subnetwork trace.
  • What do you think about adding a default phase to act as an example of what the user should expect. If swipe to delete can be enabled, the user could easily delete the default phase.
  • I know we discussed the title issue during the review but it seems a bit odd to have no title at all. What are your thoughts about adding the title "Phases, total customers, total load" or "Phase, customers per phase, load per phase"? That way, it provides a bit more context for the user and can help with the issue of displaying the units.

arcgis-ios-sdk-samples/Utility network/Create load report/README.md Outdated Show resolved Hide resolved
@yo1995 yo1995 requested a review from vquach2404 March 6, 2021 21:38
@yo1995
Copy link
Collaborator Author

yo1995 commented Mar 6, 2021

Thanks for the suggestions. I agree with and applied all of them and updated the screenshot. Please take another look!

vquach2404
vquach2404 previously approved these changes Mar 8, 2021
View reviewed changes
Copy link
Collaborator

@vquach2404 vquach2404 left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few more suggestions. Feel free to add Phil!

arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/README.md Outdated Show resolved Hide resolved
@yo1995 yo1995 requested a review from philium March 8, 2021 23:50
philium
philium reviewed Mar 9, 2021
View reviewed changes
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReport.storyboard Outdated Show resolved Hide resolved
@yo1995 yo1995 requested a review from philium March 11, 2021 03:13
philium
philium reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@philium philium left a comment

Choose a reason for hiding this comment

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

When a phase is removed, should it's summary be removed as well? Just a thought. I noticed that when I removed a phase with a summary and then re-added it, the summary was still there.

arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated
traceParameters.traceConfiguration?.traversability?.barriers = AGSUtilityTraceOrCondition(leftExpression: expression, rightExpression: phasesAttributeComparison)

traceGroup.enter()
utilityNetwork.trace(with: traceParameters) { [weak self] results, _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I don't know that one approach would be better than the others, so just stick with what you have.

arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
@yo1995
Copy link
Collaborator Author

yo1995 commented Mar 12, 2021

From Friday's review

  • The upper half of the list doesn't need to be reorder-able. Instead, making it alphabetically ordered make more sense.
  • Good to use abbreviations
  • Clear dict when removed

yo1995
yo1995 commented Mar 12, 2021
View reviewed changes
Copy link
Collaborator Author

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

Addressed a few suggestions and looking for more!

arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
arcgis-ios-sdk-samples/Utility network/Create load report/CreateLoadReportViewController.swift Outdated Show resolved Hide resolved
@yo1995 yo1995 requested a review from philium March 12, 2021 21:04
@yo1995
Copy link
Collaborator Author

yo1995 commented Mar 22, 2021

From Jennifer: need to set includeBarriers to false.

@yo1995 yo1995 added the high priority Issues that should be worked on before a release is wrapped label Mar 25, 2021
@yo1995 yo1995 requested a review from philium March 29, 2021 16:58
philium
philium approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@philium philium left a comment

Choose a reason for hiding this comment

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

🎉

saratchandrakarumuri
saratchandrakarumuri reviewed Mar 29, 2021
View reviewed changes
Copy link
Contributor

@saratchandrakarumuri saratchandrakarumuri left a comment

Choose a reason for hiding this comment

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

  • Reset button is just resetting the values in report but not changing the list of selected phases. Is this expected?
  • If the above mentioned behavior is expected, what is the purpose of even resetting those values, are they tend to change on different runs?

@yo1995
Copy link
Collaborator Author

yo1995 commented Mar 29, 2021

  • Reset button is just resetting the values in report but not changing the list of selected phases. Is this expected?

Yep it is expected. Reset button only resets the load report summaries but not the phase table.

  • If the above mentioned behavior is expected, what is the purpose of even resetting those values, are they tend to change on different runs?

In a real world use case, the value will change on each run, since the electric utility network updates in real-time. But our demo doesn't change because the demo network is static.

I didn't find a doc that describes such use case in specific, but here is a similar one with the water network.

saratchandrakarumuri
saratchandrakarumuri approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@saratchandrakarumuri saratchandrakarumuri left a comment

Choose a reason for hiding this comment

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

Sample works as expected

@yo1995 yo1995 merged commit 0375157 into v.next Mar 29, 2021
@yo1995 yo1995 deleted the Ting/Fix-SceneBasemap-vNext branch March 29, 2021 22:08
@yo1995 yo1995 removed the high priority Issues that should be worked on before a release is wrapped label Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@philium philium philium approved these changes

@saratchandrakarumuri saratchandrakarumuri saratchandrakarumuri approved these changes

@vquach2404 vquach2404 vquach2404 left review comments

Assignees

@yo1995 yo1995

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yo1995 @philium @saratchandrakarumuri @vquach2404