Feature/51 bookmarks widget #200

AndersenBell · 2023-07-18T15:40:36Z

closes #51

Adds support for the bookmark widget.
Adds missing properties on the Expander widget
Adds deprecated comments to deprecated properties
Adds Widget re-render support

seahro · 2023-07-18T17:07:08Z

src/dymaptic.GeoBlazor.Core/Scripts/jsBuilder.ts

+    if (!(dnBookmark.thumbnail == null)) {
+        //ESRI has this as an "object" with url property
+        let thumbnail = new Object();
+        thumbnail.url = dnBookmark.thumbnail;


In the future, does it make sense to give thumbnails their own class? Other than bookmarks, I would think popups and some other items could use them.

seahro · 2023-07-18T17:07:33Z

src/dymaptic.GeoBlazor.Core/Components/Widgets/ExpandWidget.cs

@@ -25,13 +26,15 @@ public class ExpandWidget : Widget

    /// <summary>
    ///     Icon font used to style the Expand button when collapsed.
+    ///     Deprecated
    /// </summary>
    [Parameter]
    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? CollapseIconClass { get; set; }


Do we want to simply remove these since they are deprecated?

No, @AndersenBell we should use the C# [Obsolete] attribute for deprecated properties.

seahro

Looks good. Good job integrating the variety of items that go into implementing each bookmark.
One question about keeping the Deprecated properties.

TimPurdum

A few odds and ends, clarifying the two samples, adding [Obsolete] to deprecated properties, etc.

TimPurdum · 2023-07-18T17:10:49Z

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Bookmarks.razor

+
+<div class="links-div">
+    <a class="btn btn-secondary" target="_blank" href="https://developers.arcgis.com/javascript/latest/api-reference/esri-widgets-Bookmarks.html">ArcGIS API for JavaScript Reference</a>
+    <a class="btn btn-primary" target="_blank" href="https://www.arcgis.com/home/item.html?id=70b726074af04a7e9839d8a07f64c039">Bookmarks widget</a>


The text on this line should say "Hurricanes", i.e., the name of the layer.

Still need to update the text here.

TimPurdum · 2023-07-18T17:13:52Z

src/dymaptic.GeoBlazor.Core/Components/Widgets/ExpandWidget.cs

@@ -25,13 +26,15 @@ public class ExpandWidget : Widget

    /// <summary>
    ///     Icon font used to style the Expand button when collapsed.
+    ///     Deprecated
    /// </summary>
    [Parameter]
    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? CollapseIconClass { get; set; }


No, @AndersenBell we should use the C# [Obsolete] attribute for deprecated properties.

TimPurdum · 2023-07-18T17:16:46Z

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Bookmarks.razor

+                    <div class="col-12">
+                        <p>Click on a bookmark to navigate to its location.</p>
+                        <BookmarksWidget  />
+                    </div>


Is this page not finished? Or does the BookmarksWidget really work with no parameters?

TimPurdum · 2023-07-18T17:17:23Z

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Widgets.razor

+    {
+        <ExpandWidget Position="OverlayPosition.TopRight" Expanded="true" ExpandIcon="refresh" CollapseIcon="submit" >
+            <BookmarksWidget @ref="_bookmarkWidget"  EditingEnabled="true">
+                @*<Bookmark Name="InlineExample">


What if we moved the inline markup example to the Bookmarks.razor page?

TimPurdum · 2023-07-18T17:33:24Z

src/dymaptic.GeoBlazor.Core/Components/Views/Viewpoint.cs

+        }
+
+    }
+


There's one more method I'd like us to get in the habit of overriding, internal override ValidateRequiredChildren(). In that, you call each "registered" child like Child.ValidateRequiredChildren(). This is what makes the [RequiredProperty] attribute work.

In this case, I don't think any of your children have required props, but it's still good if we do this everywhere.

TimPurdum · 2023-07-18T17:34:45Z

src/dymaptic.GeoBlazor.Core/Components/Widgets/ExpandWidget.cs

+}
+
+[JsonConverter(typeof(EnumToKebabCaseStringConverter<Mode>))]
+public enum Mode


this will need an xml doc comment.

TimPurdum · 2023-07-18T17:37:05Z

src/dymaptic.GeoBlazor.Core/Scripts/jsBuilder.ts

+
+    if (!(dnBookmark.thumbnail == null)) {
+        //ESRI has this as an "object" with url property
+        let thumbnail = new Object();


This should be definable as

let thumbnail = { url: dnBookmark.thumbnail };

thanks, I tried that it couldn't find the syntax to get it to work, works now.

TimPurdum

Just a few odds and ends to wrap up, looking good!

TimPurdum · 2023-07-21T19:45:17Z

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Bookmarks.razor

+    #timeSliderDiv {
+        width: 300px;
+    }
+</style>


This stuff still seems non-standard for a Blazor page. If we really need these values, they should be in Bookmarks.razor.css, probably just leftovers from testing?

this is copied from the ESRI example... so maybe?

I don't think you need the line or the <style> tag.

TimPurdum · 2023-07-21T19:47:26Z

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Bookmarks.razor

+
+<div class="links-div">
+    <a class="btn btn-secondary" target="_blank" href="https://developers.arcgis.com/javascript/latest/api-reference/esri-widgets-Bookmarks.html">ArcGIS API for JavaScript Reference</a>
+    <a class="btn btn-primary" target="_blank" href="https://www.arcgis.com/home/item.html?id=70b726074af04a7e9839d8a07f64c039">Bookmarks widget</a>


Still need to update the text here.

samples/dymaptic.GeoBlazor.Core.Sample.Shared/Pages/Bookmarks.razor

src/dymaptic.GeoBlazor.Core/Components/Layers/FeatureLayerView.cs

src/dymaptic.GeoBlazor.Core/Components/Views/Viewpoint.cs

src/dymaptic.GeoBlazor.Core/Components/Widgets/Bookmark.cs

src/dymaptic.GeoBlazor.Core/Components/Widgets/BookmarksWidget.cs

src/dymaptic.GeoBlazor.Core/Components/WebMap.cs

src/dymaptic.GeoBlazor.Core/Events/BookmarkSelectEvent.cs

src/dymaptic.GeoBlazor.Core/Scripts/jsBuilder.ts

seahro

looks great. "Effects", "Time" and "Filter" will be nice additions to built out in future iterations.

seahro and others added 29 commits June 12, 2023 20:22

initial widget class setup

985dd8f

add bookmarks to widgets.razor

c473ade

add bookmars to navmenu

6354db8

working on arcGisJsInterop

7762f2f

working on arcgisjsinterop

fa590f3

Merge branch 'develop' into feature/181_bookmarks_widget

ed44896

Merge branch 'develop' into feature/181_bookmarks_widget

1d96b96

bookmark widget .cs complete js interop working...

9e0720c

add bookmark in definitions

a02a94a

adjusting bookmark widget

71bf8d8

working on widgets page and bookmark class

501bb14

working

3364906

working on widget

040deb2

merge conflict resolved

d8859a7

bookmark page map rendering working on individual bookmarks

2dd80c8

building expand widget to hold bookmarks

d5c788c

wip bookmarks

1f92e12

working

efe1d57

updating with branch 51

bd44d4d

fixing viewpoint

f46d028

separating bookmark and viewpoint classes into individual files

7d419d7

merging 181 and 51

46ee506

working on merge conflicts

e245534

fixing some things

c996387

updating naming of class

87ba7b8

adding time extent and thumbnails

9cd95f4

Adding support for expander properties

0ce01bb

adding expand mode

6bd3769

Merge branch 'develop' into feature/51-bookmarks-widget

697d343

AndersenBell requested a review from seahro July 18, 2023 15:40

AndersenBell requested a review from TimPurdum July 18, 2023 15:40

AndersenBell self-assigned this Jul 18, 2023

seahro reviewed Jul 18, 2023

View reviewed changes

seahro approved these changes Jul 18, 2023

View reviewed changes

TimPurdum approved these changes Jul 18, 2023

View reviewed changes

AndersenBell added 4 commits July 19, 2023 08:44

code review feedback

a323b46

Merge branch 'develop' into feature/51-bookmarks-widget

cbeca17

removing bookmark page

8f3f4c3

code review feedback

8c040e2

AndersenBell requested a review from TimPurdum July 19, 2023 15:05

AndersenBell added 4 commits July 21, 2023 13:17

getting bookmark example working

445614f

Merge branch 'feature/51-bookmarks-widget' into feature/bookmark-testing

03cedb1

Merge branch 'feature/bookmark-testing' into feature/51-bookmarks-widget

25adc96

Merge branch 'develop' into feature/51-bookmarks-widget

1dcf3ce

AndersenBell requested a review from seahro July 21, 2023 19:40

TimPurdum approved these changes Jul 21, 2023

View reviewed changes

seahro approved these changes Jul 21, 2023

View reviewed changes

AndersenBell added 2 commits July 21, 2023 15:03

code review feedback

4c28e8a

fixing comment

737b14e

AndersenBell merged commit 0ca7b69 into develop Jul 21, 2023

AndersenBell deleted the feature/51-bookmarks-widget branch July 21, 2023 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/51 bookmarks widget #200

Feature/51 bookmarks widget #200

AndersenBell commented Jul 18, 2023

seahro Jul 18, 2023

seahro Jul 18, 2023

TimPurdum Jul 18, 2023

seahro left a comment

TimPurdum left a comment

TimPurdum Jul 18, 2023

TimPurdum Jul 21, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

TimPurdum Jul 18, 2023

AndersenBell Jul 19, 2023

TimPurdum left a comment

TimPurdum Jul 21, 2023

AndersenBell Jul 21, 2023

TimPurdum Jul 21, 2023

TimPurdum Jul 21, 2023

seahro left a comment

Feature/51 bookmarks widget #200

Feature/51 bookmarks widget #200

Conversation

AndersenBell commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seahro left a comment

Choose a reason for hiding this comment

TimPurdum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimPurdum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seahro left a comment

Choose a reason for hiding this comment