Skip to content

Conversation

@ktyle
Copy link
Contributor

@ktyle ktyle commented May 27, 2021

I've reworked the intro to cartopy notebook so it fits the proposed foundations template that @dcamron developed on the 5/27 hackathon

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 3d88327 at: https://60b005e29c6ddd1fd50c9665--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

I think to fit the template, we need to avoid using subheadings for text blocks, particularly because they clutter up the right-hand nav bar when rendered in the Jupyterbook.

I'll push an update to this branch that has a more vanilla usage of subheading markers.

@brian-rose
Copy link
Member

@ktyle I removed a lot of subheading markers in text blocks, but also tried to add useful subheadings that will render well in the nav bar. Let me know what you think.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: dbd5f72 at: https://60b0192b729f6c41ac807050--pythia-foundations.netlify.app

@brian-rose brian-rose added the content Content related issue label May 28, 2021
@brian-rose
Copy link
Member

This should probably be revisited one more time as soon as the template is finalized and merged.

@clyne clyne added this to the InitialPortalRelease milestone Jun 4, 2021
@github-actions
Copy link

github-actions bot commented Jun 8, 2021

🚀 📚 Preview for git commit SHA: 0b9ffdd at: https://60bf8e82fd59d63b5ea342a4--pythia-foundations.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

🚀 📚 Preview for git commit SHA: ab35744 at: https://60bf910c3bced800994c9b07--pythia-foundations.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

🚀 📚 Preview for git commit SHA: e6693ba at: https://60bf94d0ec5aa200b0444565--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

Ok team, I did some further massaging of this notebook to make it (I think) fully consistent with our new template. I also added the self-generating Cartopy logo at the top of the notebook using code from https://scitools.org.uk/cartopy/docs/latest/gallery/miscellanea/logo.html

@ktyle is this ready for a formal review? If so, please request from the Education team. (I see that you "assigned" some of us to this already, feel free to tag the same people for review or tag the team instead to use the round-robin functionality)

@ktyle ktyle requested review from a team, brian-rose and r-ford and removed request for a team June 8, 2021 17:45
@ktyle
Copy link
Contributor Author

ktyle commented Jun 8, 2021

@brian-rose looks great! Ready for review and merge. I selected the Education team so hopefully the reviewers got assigned properly.

@brian-rose brian-rose requested a review from dcamron June 8, 2021 18:33
@brian-rose brian-rose requested a review from mgrover1 June 8, 2021 18:33
@brian-rose
Copy link
Member

Ok, I manually added @dcamron and @mgrover1 as reviewers since the GitHub machine chose me (I've already worked over the content) and @r-ford who isn't yet active on the project.

@brian-rose
Copy link
Member

Side note, we should (temporarily) remove @r-ford from the team so he doesn't get tagged for reviews during tomorrow's hackathon.

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:22Z
----------------------------------------------------------------

Can you separate the cartopy.feature as cfeature into a different line?


ktyle commented on 2021-06-08T19:03:56Z
----------------------------------------------------------------

That's how I originally had it, and I prefer it that way too! But as I recall it got flagged by the linter in favor of this usage.

mgrover1 commented on 2021-06-08T21:08:28Z
----------------------------------------------------------------

Ahh okay!

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:23Z
----------------------------------------------------------------

there is a weird space here in the rendering


ktyle commented on 2021-06-09T15:35:43Z
----------------------------------------------------------------

Ok I removed the extraneous space preceding the final sentence. I also added a clarification that ~ represents one's home directory.

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:24Z
----------------------------------------------------------------

Consider making this a hyperlink?


ktyle commented on 2021-06-09T15:38:03Z
----------------------------------------------------------------

Hmm it already is ... do you mean that the text within the link should say something other than the full URL?

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:24Z
----------------------------------------------------------------

Hyperlink here too


ktyle commented on 2021-06-09T15:38:33Z
----------------------------------------------------------------

same as previous comment

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:25Z
----------------------------------------------------------------

Is there a better name we could use here? Maybe labels?


ktyle commented on 2021-06-09T15:39:35Z
----------------------------------------------------------------

Do you mean substitute Info with Labels?

mgrover1 commented on 2021-06-09T15:56:36Z
----------------------------------------------------------------

Yes - I think that would be more informative

ktyle commented on 2021-06-09T16:03:42Z
----------------------------------------------------------------

(resolved; this issue was a consequence of how reviewNB renders some Markdown cells, such as the alert admonitions)

@review-notebook-app
Copy link

review-notebook-app bot commented Jun 8, 2021

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2021-06-08T18:42:26Z
----------------------------------------------------------------

Why is #3 italicized?


ktyle commented on 2021-06-09T15:41:04Z
----------------------------------------------------------------

Fixed :)

Copy link
Contributor

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

I added a few comments/suggestions via nbviewer - the markdown file looks great. Awesome work!

Copy link
Contributor Author

ktyle commented Jun 8, 2021

That's how I originally had it, and I prefer it that way too! But as I recall it got flagged by the linter in favor of this usage.


View entire conversation on ReviewNB

Copy link
Contributor

mgrover1 commented Jun 8, 2021

Ahh okay!


View entire conversation on ReviewNB

Copy link
Contributor Author

ktyle commented Jun 9, 2021

Ok I removed the extraneous space preceding the final sentence. I also added a clarification that ~ represents one's home directory.


View entire conversation on ReviewNB

Copy link
Contributor Author

ktyle commented Jun 9, 2021

Hmm it already is ... do you mean that the text within the link should say something other than the full URL?


View entire conversation on ReviewNB

Copy link
Contributor Author

ktyle commented Jun 9, 2021

same as previous comment


View entire conversation on ReviewNB

Copy link
Contributor Author

ktyle commented Jun 9, 2021

Do you mean substitute Info with Labels?


View entire conversation on ReviewNB

Copy link
Contributor Author

ktyle commented Jun 9, 2021

Fixed :)


View entire conversation on ReviewNB

Copy link
Contributor

mgrover1 commented Jun 9, 2021

Yes - I think that would be more informative


View entire conversation on ReviewNB

@mgrover1 mgrover1 self-requested a review June 9, 2021 15:59
Copy link
Contributor Author

ktyle commented Jun 9, 2021

(resolved; this issue was a consequence of how reviewNB renders some Markdown cells, such as the alert admonitions)


View entire conversation on ReviewNB

@andersy005 andersy005 added the hackathon Issue highlighted for active hackathon session label Jun 9, 2021
@github-actions
Copy link

github-actions bot commented Jun 9, 2021

🚀 📚 Preview for git commit SHA: c71c9c7 at: https://60c0e89cb198cb00b42bfe29--pythia-foundations.netlify.app

Copy link
Contributor

@mgrover1 mgrover1 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 making those changes @ktyle looks good to merge!

@mgrover1 mgrover1 merged commit 65807cf into ProjectPythia:main Jun 9, 2021
@ktyle
Copy link
Contributor Author

ktyle commented Jun 9, 2021

woo-hoo!

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

Labels

content Content related issue hackathon Issue highlighted for active hackathon session

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants