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

Update PULL_REQUEST_TEMPLATE.md #1505

Merged
merged 5 commits into from
Jul 20, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
<!--
IMPORTANT: We applaud pull requests, they excite us every single time. As we have an obligation to maintain a healthy code standard and quality, we take considerate amounts of time for reviews.
-->

**Fixes #0000**

**Changes proposed in this pull request:**
<!-- fill this out -->
<!-- fill this out, mention the pages and/or components which have been impacted -->

**Reviewers should focus on:**
<!-- fill this out -->
<!-- fill this out, ask for feedback on specific changes you are unsure about -->

**Screenshot**
<!-- include an image of the most relevant user-facing change, if any -->

**Confirmed**

- [ ] for visual or frontend changes that the changes work as intended via visual confirmation on a local flarum installation.
- [ ] for php changes that the unit tests are all AOK (green) by running them locally (`php vendor/bin/phpunit`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay for the checklist!

Can we make these a bit shorter, though?

  • frontend changes: tested on a local Flarum installation
  • backend changes: tests are green (run php vendor/bin/phpunit)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.


**Required changes:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these meant for suggested follow-ups (e.g. docs) or for dependent, but existing pull requests in other repos (e.g. extensions)? It wasn't clear to me...

Copy link
Member Author

Choose a reason for hiding this comment

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

line 21 is, the lines above that are meant as checks/requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is:

  • "Docs" sounds like more of a TODO for later,
  • "Core extensions" seems to be the place for links to related PRs.

Or should both contain links to related PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally docs should be PR'd at the same time (at least once we have a good foundation of docs!)

Copy link
Contributor

Choose a reason for hiding this comment

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

So can we just combine these two with the list above?

  • Related documentation PR: (Remove if irrelevant)
  • Related extension PRs: (Remove if irrelevant)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice one.


- **Docs**: <!-- list of subjects or pages -->
- **Core extensions**: <!-- list of extensions -->