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

Adding measurement widget #196

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Adding measurement widget #196

merged 3 commits into from
Jul 19, 2023

Conversation

AndersenBell
Copy link
Collaborator

closes #58

Adds measurement widget to the map

@AndersenBell AndersenBell requested review from seahro and TimPurdum July 13, 2023 20:41
@AndersenBell AndersenBell self-assigned this Jul 13, 2023
/// </summary>
[Parameter]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? Icon { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be in the MeasurementWidget.cs? I saw that the both have Icon properties. Just wanted to verify.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should be removed from the measurement widget and inherited from the widget class

Copy link
Collaborator

@seahro seahro 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. One question on the Icon property that is in widget.

Copy link
Collaborator

@TimPurdum TimPurdum left a comment

Choose a reason for hiding this comment

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

Looking good, just a few requests!

/// </summary>
public DotNetObjectReference<MeasurementWidget> MeasurementWidgetObjectReference => DotNetObjectReference.Create(this);

[Parameter]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need xml comments on all public types for the doc generation.

/// </a>
/// </summary>
[JsonConverter(typeof(EnumToKebabCaseStringConverter<AreaUnit>))]
public enum AreaUnit
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a better way to identify the units used in GeometryEngine vs. the ones used in the new widget. Since they changed the names on me anyways, could we create a new GeometryEngineAreaUnit, create overloads in GeometryEngine.cs that take those instead, and mark the ones that take ArealUnit as [Obsolete]?

linearUnit: widget.linearUnit,
label: widget.label,
icon: widget.icon,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will break if you don't set all the values in the sample. The pattern I suggest is activeTool: widget.activeTool ?? undefined. This prevents setting null values in arcgis, which are sometimes not expected or handled.

@AndersenBell AndersenBell requested a review from TimPurdum July 18, 2023 20:46
Copy link
Collaborator

@TimPurdum TimPurdum left a comment

Choose a reason for hiding this comment

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

Nicely done!

/// <a target="_blank" href="https://developers.arcgis.com/javascript/latest/api-reference/esri-widgets-Measurement.html">
/// ArcGIS
/// JS API
/// </a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

you were right about this formatting, I solved it another way by fixing it in docCopy.ps1, so we can let the XML formatter do its thing.

@TimPurdum TimPurdum merged commit 7b9e98c into develop Jul 19, 2023
@TimPurdum TimPurdum deleted the feature/58_measurement_widget branch July 19, 2023 20:26
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.

Add Measurement Widget
3 participants