Skip to content

Change Upcoming Events to Recent Events#2

Merged
DaveSauce merged 2 commits into
masterfrom
gleb-recent-events
Dec 17, 2020
Merged

Change Upcoming Events to Recent Events#2
DaveSauce merged 2 commits into
masterfrom
gleb-recent-events

Conversation

@GlebBabahov
Copy link
Copy Markdown

No description provided.

@GlebBabahov GlebBabahov self-assigned this Dec 10, 2020
I decided to include it, since we should all be on the same version of everything, but this may cause dev issues later
@DaveSauce
Copy link
Copy Markdown

Thanks for the PR. I have a couple of notes:

  • Your code looked good - simple and small pull request, it was easy to read.

  • That said, it actually should have been two separate commits and pull requests, since adding gatsby-cli and updating the text on the home page are separate concerns, and don’t depend on each other.

  • There was a conflict with the master branch that I had to resolve. Make sure the branch is clear from conflicts when creating the PR - I won’t fix these for you in the future

  • Your commit message was good (at least re: the text update), especially since the PR was so short. For a larger commit, you will likely need more test. Here’s an article that has some good thoughts on commit messages (we don’t have to do it exactly like they suggest, but it will give you a good sense)

  • Next time, please be sure to include a detailed explanation in the pull request. This will be much more important when there are multiple commits to summarize. Be sure to include a summary of the changes, why they were made, and how to test the changes. Don’t be shy with screen shots and links to issues or anything else that’s relevant. https://github.blog/2015-01-21-how-to-write-the-perfect-pull-request/

@DaveSauce DaveSauce merged commit cc28402 into master Dec 17, 2020
@DaveSauce DaveSauce deleted the gleb-recent-events branch December 17, 2020 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants