-
Notifications
You must be signed in to change notification settings - Fork 85
chore(template): add pull request template #43
Conversation
📊 Bundle Size Report
|
.github/pull_request_template.md
Outdated
- [ ] This change impacts HTTP headers. | ||
- [ ] This change has new infrastructure requirements. | ||
- [ ] This change requires additional environment variables. | ||
- [ ] I have added the Apache 2.0 license to any new files created. |
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.
Should be license header
.
.github/pull_request_template.md
Outdated
- [ ] This change requires a performance test prior to merging. | ||
- [ ] This change impacts caching. | ||
- [ ] This change impacts HTTP headers. | ||
- [ ] This change has new infrastructure requirements. |
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 should be explained in more detail
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.
Yes it should. Any input here as it was a question in the preflight checklist and wasn't sure.
.github/pull_request_template.md
Outdated
- [ ] I have added the Apache 2.0 license to any new files created. | ||
|
||
## What is the Impact to Developers Using the App? | ||
<!--- Please describe how your changes impacts developers using the app. --> |
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.
s/app/One App
.github/pull_request_template.md
Outdated
- [ ] These changes should be applied to a maintenance branch. | ||
- [ ] This change requires cross browser checks. | ||
- [ ] This change requires a performance test prior to merging. | ||
- [ ] This change impacts caching. |
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.
Should specify that you mean caching for client browsers.
.github/pull_request_template.md
Outdated
- [ ] I have updated the documentation accordingly. | ||
- [ ] These changes should be applied to a maintenance branch. | ||
- [ ] This change requires cross browser checks. | ||
- [ ] This change requires a performance test prior to merging. |
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.
You should specify if you mean client side of server side perf test.
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.
What do we mean in the current Preflight Checklist?
.github/pull_request_template.md
Outdated
- [ ] This change impacts caching. | ||
- [ ] This change impacts HTTP headers. | ||
- [ ] This change has new infrastructure requirements. | ||
- [ ] This change requires additional environment variables. |
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.
How about This change adds additional environment variable requirements for One App users
?
e72af28
to
86d7591
Compare
This is much more reliable and better state management
.on('close', () => res(bytesWritten)) | ||
.pipe(byteCounter) | ||
.on('error', rej); | ||
pipeline( |
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 suggestion actually came from James Snell. He is on the Node.js TSC.
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.
Nice name drop lol
Co-Authored-By: Jimmy King <jimmy.king@aexp.com>
This was taken care of already
This adds a pull request template for newly created PRs to get more information about the PR being opened.