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 CSV Layer #169

Merged
merged 8 commits into from
Jul 13, 2023
Merged

Adding CSV Layer #169

merged 8 commits into from
Jul 13, 2023

Conversation

AndersenBell
Copy link
Collaborator

@AndersenBell AndersenBell commented May 12, 2023

Closes #65

Adding support for the CSV Layer

The sample pulls the sample CSV from the developer ArcGIS portal

It follows the GEOJson structure, so it sets the renderer and spatial reference in the interop js file

@AndersenBell AndersenBell requested a review from TimPurdum May 12, 2023 19:36
@AndersenBell AndersenBell self-assigned this May 12, 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.

Thanks for doing this! I think we need some new gissues to investigate properties and methods that are not implemented here, and on some other common layer types like GeoJSON, but are in the ArcGIS spec. If you have time to write up some of those, feel free. An example of how I've done that is #117.

Request changes mostly just for the dedicated sample page.

Comment on lines +48 to +49
Url = url;
Title = title;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize this is missing in a lot of classes, including most layers, but we should wrap these parameter setters in #pragma warning disable BL0005. Search for that and you'll see some examples where I've implemented it. If you don't do this, and you create the class in C# with this constructor, you will get a warning about setting parameters outside of markup.

@@ -8,6 +8,7 @@
<button disabled="@(!_mapRendered)" @onclick="() => ToggleLayer(_graphicsLayer)">Graphics Layer</button>
<button disabled="@(!_mapRendered)" @onclick="() => ToggleLayer(_geoJsonLayer)">GeoJSON Layer</button>
<button disabled="@(!_mapRendered)" @onclick="() => ToggleLayer(_geoRssLayer)">GeoRSS Layer</button>
<button disabled="@(!_mapRendered)" @onclick="() => ToggleLayer(_csvLayer)">CSV Layer</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you create a full samples page for this new layer type? It helps with the discoverability, and this tests page is a mess that I don't publish to our samples.geoblazor.com site.

@AndersenBell AndersenBell requested a review from TimPurdum July 11, 2023 21:36
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.

Looks great!


private async Task AddLayer()
{
if (!string.IsNullOrEmpty(_csvLayerUrl))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the original CSV layer when adding a new one? Did you try binding to the <CSVLayer Url="_csvLayerUrl"> value? I'm sure there could be issues with that, you might have to call mapView.Refresh().

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 didn't try it. I was trying to include multiple examples of adding a csv layer, so if you look at the source you can see how to do it declaratively and programmatically.

If(!(test-path -PathType container $path))
{
New-Item -ItemType Directory -Path $path
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding this!

/// coordinate is the Y value, and the longitude coordinate is the X value. The X, Y coordinates must be stored
/// in SpatialReference.WGS84 in csv feed.
/// <a target="_blank" href="https://developers.arcgis.com/javascript/latest/api-reference/esri-layers-CSVLayer.html">
/// ArcGIS
Copy link
Collaborator

Choose a reason for hiding this comment

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

These line breaks cause issues with our docs generation, I believe. We might need to inline this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I think it was auto-formatted this way

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 looks like its formatted exactly like other layers in the project

@TimPurdum TimPurdum merged commit f3eb99d into develop Jul 13, 2023
@TimPurdum TimPurdum deleted the feature/65_add_csv_layer branch July 13, 2023 17:34
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 a CSVLayer
2 participants