Skip to content

Conversation

@jukent
Copy link
Contributor

@jukent jukent commented Mar 2, 2022

GH branches has overlap from GH pull requests. I'm shortening the merging branches section and pointing to the PR chapter.

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

This pull request is being automatically built with GitHub Actions and Netlify. To see the status of your deployment, click below.

🔍 Git commit SHA: ab5523d
✅ Deployment Preview URL: https://625d7cfc7e5f4f556b8fcded--pythia-foundations.netlify.app

@jukent jukent marked this pull request as ready for review March 2, 2022 19:14
@jukent jukent requested a review from a team as a code owner March 2, 2022 19:14
@jukent jukent requested review from brian-rose and mgrover1 and removed request for a team March 2, 2022 19:14
@jukent jukent requested review from clyne and ktyle March 2, 2022 20:14
@jukent jukent self-assigned this Mar 2, 2022
@brian-rose
Copy link
Member

Yeah, I agree with @jukent here. The fork-based workflow is that you push commits to a branch on your fork and then you merge your fork's branch into the upstream main. You should not be merging your fork's branch into your fork's main.

Agreed +1

In this workflow, the only changes you should be making to your main branch is merging changes from upstream main.

@clyne
Copy link
Contributor

clyne commented Apr 14, 2022

The slides were quick-and-dirty and intended to be exemplars. Feel free to modify to better reflect reality as you see fit :-)

@jukent
Copy link
Contributor Author

jukent commented Apr 14, 2022

@clyne @kmpaul and I just spent some time iterating on clearer slides. This PR should now reflect the new images and gifs we made. Once it has built, I will re-request your review.

https://docs.google.com/presentation/d/1_0xuSQ9G27kkMxrcJX4eICy8j1j7vmYg0tYCrpFnpto/edit?usp=sharing

@jukent jukent changed the title Removing PR content from GH branch chapter Polishing GH branch chapter Apr 14, 2022
@jukent jukent requested a review from kmpaul April 14, 2022 22:57
@jukent
Copy link
Contributor Author

jukent commented Apr 14, 2022

@clyne @brian-rose This is ready for review

@kmpaul
Copy link
Contributor

kmpaul commented Apr 14, 2022

@jukent: This is looking great! Thanks for all the work on this.

Some nit-picky details that I will leave to you to decide whether to act on:

I think the GIF animations might need to be modified so that people know where in the "slide deck" the image is. So, maybe a "slide number" in a corner somewhere so that people know where they are.

Also, I think that some of the GIF animations are just too slow. I think it would be better for them to be faster and just on an infinite loop.

Also, I wonder if we can lengthen the start and end delays of the animations. That is, if the "next slide" time is 2 seconds, then maybe don't change the first slide until after 3 seconds and don't loop after the last slide until 3 seconds. ...? I'm not sure how to do this. Although, you could accomplish it be having duplicate slides and a faster "next slide" time, like so. If you have slides A, B, and C, you might create a "slide deck" with:

A -> A -> A -> B -> B -> C -> C -> C

And with a 1 second transition time, that would result in slide A being displayed for 3 seconds, B for 2 seconds, and C for 3 seconds. ...Just a thought. But maybe that is too picky.

Lastly, I think the slides need to be cropped. The whitespace above and below the images is too large.

@jukent
Copy link
Contributor Author

jukent commented Apr 14, 2022

@kmpaul just added slide numbers, sped up the animation, and cropped the gifs.

Copy link
Contributor

@clyne clyne 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, Julia. I've added a couple minor text edits to consider.

@jukent jukent requested a review from clyne April 18, 2022 14:52
@jukent
Copy link
Contributor Author

jukent commented Apr 18, 2022

Looks great, Julia. I've added a couple minor text edits to consider.

Thanks! These changes are incorporated now.

Copy link
Contributor

@clyne clyne 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, @julia!

Is the source material for your new figures available? I'd like to reuse for the section on PRs. Thanks!

@jukent
Copy link
Contributor Author

jukent commented Apr 18, 2022

Is the source material for your new figures available? I'd like to reuse for the section on PRs. Thanks!

Thanks!

https://docs.google.com/presentation/d/1_0xuSQ9G27kkMxrcJX4eICy8j1j7vmYg0tYCrpFnpto/edit?usp=sharing

Copy link
Contributor

@kmpaul kmpaul left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @jukent!

@jukent jukent merged commit e8fcc97 into ProjectPythia:main Apr 18, 2022
@jukent jukent deleted the branchPR branch April 18, 2022 15:42
@brian-rose brian-rose added this to the GitHub content milestone Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

content Content related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants