-
Notifications
You must be signed in to change notification settings - Fork 1.1k
intro to briefings guide 3/4 #2442
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
base: next
Are you sure you want to change the base?
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
kgalliher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of minor grammatical and formatting suggestions. The final cells don't work for me but I'm not sure that's a dealbreaker.
| "source": [ | ||
| "### Concept of slides and blocks\n", | ||
| "\n", | ||
| "Similar to traditional presentations with slides, `Briefing`s come with `BriefingSlide`s to add structured content to your Briefing.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Briefings need to be code highlighted?
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good way to start. However, as a new user I would not understand where these classes are coming from or where the Image class popped up from and why you are creating it.
I would add the same intro as for the StoryMap content guide where you list all the available content classes for a Briefing. Since there are also Briefing specific classes those should be included and detailed here as well.
Users understand Briefings in a storymap editor context but that understanding needs to be mapped here since this could be the first time they are seeing any of this. (most likely is if they are looking at the intro guide).
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this with the same intro as the StoryMap guide, however are any of these content elements not supported for Briefing?
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talk about Slides, talk about accessing the slides list, talk about how you access the cover and assign it to a variable to be edited, etc...
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would mention the themes enum class and how to see the values, also talk about the theme method and how you can assign a custom theme as well.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that intro with a visual for what these themes look like
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would mention since we are saving for the first time you are assigning a title as well. Mention how saving is important after making changes since this is what pushed the changes back to the item in the organization
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above where Text came out of nowhere for users who have not done the intro to Storymap content guide. I would mention same things you mention in that guide here as well.
This makes guides richer and users don't have to go back and forth between them to understand the concepts.
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a title to the slide to make it look cleaner since this is not a titleless slide
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can title for a slide be set using the following code ?
slide2.title = Text( text="Visual Glimpses", style=TextStyles.HEADING, )
I tried with both this and providing the str value 'Visual Glimpses' but it throws a ValueError.
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify this is a WebMap item we are grabbing from our organization to add as a Map content to the Briefing.
Reply via ReviewNB
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments on what you are doing with each cell. Or at very least add comments to the cell if markdown is not necessary
Reply via ReviewNB
| @@ -0,0 +1,858 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nanaeaubry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more info for everything. If markdowns are not present I think comments can be added to the code cells to make it clearer.
Users looking at this guide are more likely than not complete newbies to the module
Guide 3 of 4, introduction to briefings.
Closes https://github.com/ArcGIS/geosaurus/issues/12370
Checklist
Please go through each entry in the below checklist and mark an 'X' if that condition has been met. Every entry should be marked with an 'X' to be get the Pull Request approved.
imports are in the first cell?arcgisimports? Note that in some cases, for samples, it is a good idea to keep the imports next to where they are used, particularly for uncommonly used features that we want to highlight.GISobject instantiations are one of the following?gis = GIS()gis = GIS('home')orgis = GIS('pro')gis = GIS(profile="your_online_portal")gis = GIS(profile="your_enterprise_portal")./misc/setup.pyand/or./misc/teardown.py?api_data_owneruser?api_data_owneraccount and change the notebook to first download and unpack the files.<img src="base64str_here">instead of<img src="https://some.url">? All map widgets contain a static image preview? (Callmapview_inst.take_screenshot()to do so)os.path.join()? (Instead ofr"\foo\bar",os.path.join(os.path.sep, "foo", "bar"), etc.)Export Training Data Using Deep Learningtool published on geosaurus org (api data owner account) and added in the notebook usinggis.content.getfunction?gis.content.getfunction? Note: This includes providing test raster and trained model.