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

Feature/64 add kmllayer #199

Merged
merged 22 commits into from
Jul 21, 2023
Merged

Feature/64 add kmllayer #199

merged 22 commits into from
Jul 21, 2023

Conversation

seahro
Copy link
Collaborator

@seahro seahro commented Jul 17, 2023

Closes #64

Adds KML Layers to the layer types that can be added/used in geoblazor.

sample page could be consolidated in the future with other layer types.

@seahro seahro added enhancement New feature or request geoblazor labels Jul 17, 2023
@seahro seahro requested review from TimPurdum and AndersenBell July 17, 2023 22:26
@seahro seahro self-assigned this Jul 17, 2023
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.

Nice and clean, great start!


namespace dymaptic.GeoBlazor.Core.Components.Layers;

public class KMLLayer : Layer
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 and properties for the docs site.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comments are now in place after the class was refactored

kmlLayer = new KMLLayer({ portalItem: portalItem });
}
newLayer = kmlLayer;
copyValuesIfExists(layerObject, kmlLayer, 'sublayers', 'blendMode', 'maxScale', 'minScale', 'title', 'visible');
Copy link
Collaborator

Choose a reason for hiding this comment

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

copyValuesIfExists is shorthand for everything you have below it, so you can delete lines 2038-2049. The only reason to not use copyValuesIfExists is if you need to transform the value somehow, such as using a buildJS function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed and understand, there are no transformations at this time, but enabling editing may require this in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can evaluate each property individually, if there is something that needs to be spelled out, just leave it out of the copyValues list and implement it fully.


[Parameter]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? Url { 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.

Add the [RequiredProperty] attribute to Url and PortalItem (each one should reference the other one). see FeatureLayer for the example. This should throw an error if a user forgets to set either a Url or a PortalItem with Id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored the constructor, included error and added Required Property to Url.

public async Task AddKMLLayer()
{
_visible = true;
await LoadKMlLayer();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're defining the KML layer twice, once in markup on line 17, and separately in C# on line 37. So you have added the same layer twice. I know @AndersenBell was suggesting that we try to show off the different ways of adding things, but I think this can be confusing and imply that you have to do both, and it would be more clear if they were separate layers, possibly triggered by separate buttons, like Add Layer in Markup and Add Layer in Code.

@seahro seahro requested a review from TimPurdum July 19, 2023 18:00
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 bit more to implement.

kmlLayer = new KMLLayer({ portalItem: portalItem });
}
newLayer = kmlLayer;
copyValuesIfExists(layerObject, kmlLayer, 'sublayers', 'blendMode', 'maxScale', 'minScale', 'title', 'visible');
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can evaluate each property individually, if there is something that needs to be spelled out, just leave it out of the copyValues list and implement it fully.

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
[RequiredProperty(nameof(PortalItem))]
public string? Url { 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.

OK, so your code here and in the TypeScript says you are supporting PortalItem, but you haven't implemented it.

You need

  1. A new public PortalItem? PortalItem { get; set; }. NOT a [Parameter]
  2. Implement protected override async Task RegisterChildComponent(MapComponent child)
  3. Implement protected override async Task UnRegisterChildComponent
  4. Implement internal void ValidateRequiredChildred().

This is how nested markup components get registered to work. You could create a test in Core.Test.Blazor or a separate sample that lays out a KML like this

<KmlLayer>
    <PortalItem Id="..." />
</KmlLayer>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored the main class and adjusted the markup component to use the url parameter. Also created a layer test class for the kml layer and successfully tested. Thanks for the assist @TimPurdum Additional layers can also be added in the future to this test file.

@seahro seahro requested a review from TimPurdum July 21, 2023 16:39
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.

Hook up the new samples page to the nav menu. Nice work on this!

@@ -0,0 +1,53 @@
@page "/kmllayers"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this page added to navMenu.razor? I don't see it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected that oversight


<div class="links-div">
<a class="btn btn-secondary" target="_blank" href="https://developers.arcgis.com/javascript/latest/api-reference/esri-layers-KMLLayer.html">ArcGIS API for JavaScript</a>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a second link for the KML Layer source (see other sample pages for example).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

source url is now included and reformatted the page to display better.

<Map ArcGISDefaultBasemap="dark-gray-vector">
@if (_markup)
{
<KMLLayer Url="https://earthquake.usgs.gov/fdsnws/event/1/query?format=kml&minmagnitude=5.8" Title="Major EarthQuakes Query Result"></KMLLayer>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have this url string as a field already, you could just set it here with <KMLLayer Url="@_kmlLayerUrl" Title="Major Earthquakes Query Result" />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected

CreateViewRenderedHandler(async () =>
{
await AssertJavaScript("assertKmlLayerExists");
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

indent formatting is a bit wonky

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 think this was a copy paste oversight when bringing this file in after the new test folder was created. good to go

if (layers !== 'kml') {
throw new Error(`There are no Layers in this view`);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually simpler than the code I used to find the widgets, so nice work!

@TimPurdum TimPurdum merged commit 147cada into develop Jul 21, 2023
@TimPurdum TimPurdum deleted the feature/64_add_kmllayer branch July 21, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request geoblazor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a KMLLayer
2 participants