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

Using and updating GIS content sample notebook revamped according to the new map widget #2160

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tarunkukreja003
Copy link
Collaborator

Using and updating GIS content sample notebook is now revamped according to the new map widget

Major changes include:

  1. The JSON of the web scene or web map is not changed anymore, instead, we use property setters to update the web maps and web scenes
  2. Properties and methods updated according to the new map widget

NOTE
PLEASE TEST AGAINST THIS PR https://github.com/ArcGIS/geoserpent/pull/190, THE NOTEBOOK CELLS WILL ONLY WORK IF THE CHANGES FROM THIS PR IS PULLED IN

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:01Z
----------------------------------------------------------------

  • If you want to de-capitalize ArcGIS in the second sentence then I would suggest:

'The arcgis package includes several classes to make use of this content'

  • Also it was correct with this content. If you want to use 'these' then make content plural: these contents

  • Unnecessary 'the' added before them. Please remove.

Suggested full second sentence:

"The arcgis package includes several classes to make use of this content, publish new items and update them when needed."


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:02Z
----------------------------------------------------------------

Not sure if this is just in notebook reviewer but the table of contents looks super spread out


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:03Z
----------------------------------------------------------------

This seems to not be needed


jyaistMap commented on 2024-11-26T19:46:47Z
----------------------------------------------------------------

I Agree with Nanae. Please remove this cell or explain why you are searching for a WebMap of this title, and why you are searching outside the organization.

Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:04Z
----------------------------------------------------------------

If you are going to add this logic, add a description of what you are doing. This is not intuitive to new users


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:04Z
----------------------------------------------------------------

Map(item=wm_item)

The first parameter is location. Although there is a check in place let's promote best practice in samples


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:05Z
----------------------------------------------------------------

Add back this comment above this cell:

The web map was sucessfully overwritten with correct operational layers. You can interact with the widget and zoom into the USA to observe the locations of capitals.

Then inside the cell add back this comment:

# Let's clean it up so we can always run this notebook again

This way users know why we are deleting everything


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:06Z
----------------------------------------------------------------

Why don't you delete the layers instead? I don't think we want to encourage overwriting the layers property since this is not a documented workflow.

Use: map.content.delete(<layer_index>)


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:07Z
----------------------------------------------------------------

Layers property should not be set


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:08Z
----------------------------------------------------------------

There is a property for Scene called basemaps3d . It would be cool to use one of those since scene is trying to promote using them.


Copy link

review-notebook-app bot commented Nov 13, 2024

View / edit / reply to this conversation on ReviewNB

nanaeaubry commented on 2024-11-13T08:56:09Z
----------------------------------------------------------------

Add a comment how using the save method will create a new Scene or Map in portal whereas the update method we used above will update the changes to the existing Map.


Copy link
Contributor

@nanaeaubry nanaeaubry left a comment

Choose a reason for hiding this comment

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

Left some comments in the notebook reviewer. Good start with this! :)

Copy link
Collaborator

I Agree with Nanae. Please remove this cell or explain why you are searching for a WebMap of this title, and why you are searching outside the organization.


View entire conversation on ReviewNB

Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:22Z
----------------------------------------------------------------

The precedence for hashmark headings is as follows:

  • single hashmark (#) - Reserved for the title cell
  • double hashmark (##) - Reserved for right-hand navigation headings
  • triple hashmark (###) - Reserved for indented sub-headings under a double hashmark heading

Please change the markdown to a double hashmark


The current navigation does not reflect the whole notebook:

image

Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:23Z
----------------------------------------------------------------

Please remove "This seems a bit weird to put in a sample" and change it to something like "This may seem counterintuitive, but we'll delete this to demonstrate...."


Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:24Z
----------------------------------------------------------------

Change the markdown heading to a double hashmark


Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:25Z
----------------------------------------------------------------

Please add a markdown cell prefaced with double hashmarks to create a section heading for changing the basemap. You should also explain how to print out the current basemap, and then why are you are changing it. It seems changing the basemap serves no purpose.


Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:25Z
----------------------------------------------------------------

Please explain that you are searching the BasemapManager to return the list of basemaps available.


Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:26Z
----------------------------------------------------------------

Explain to users that you are changing the basemap and why. Is it just for appearance? To show how to use the basemap setter?

Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:27Z
----------------------------------------------------------------

Change the markdown to be a triple hashmark so it is a subheading of Using and updating a web map


The right-hand nav should look like this for this section:
image

Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:28Z
----------------------------------------------------------------

Please explain to the user that this indicates a content layer in the map, but no features appear on the web map. You need to replace the layer.


Copy link

review-notebook-app bot commented Nov 26, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-26T20:24:29Z
----------------------------------------------------------------

Change the markdown to be a triple hashmark so it is a subheading of Using and updating a web map


Copy link

review-notebook-app bot commented Nov 27, 2024

View / edit / reply to this conversation on ReviewNB

jyaistMap commented on 2024-11-27T00:17:46Z
----------------------------------------------------------------

It would be much more readable to set the basemap to the tite:

new_web_scene_obj.basemap.basemap = "dark-gray-vector"


Copy link
Collaborator

@jyaistMap jyaistMap left a comment

Choose a reason for hiding this comment

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

  • more explanation needs added
  • we need to look at the scene section and examine what @nanaeaubry has said about how we shouldn't be setting layers or promoting workflows we don't want used
  • markdown cell headings need to follow the appropriate syntax to build the right hand navigation:
    • # - Only for the overall title of the notebook - Using and updating GIS content in the graphic below

image

  • ## - For top level headings in the right-hand navigation - For example, Creating a web map and feature layer and Using and updating a web map in the graphic below
  • ### - For subheadings - For example, Create a feature layer and Make a copy of the public web scene item in the graphic below

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants