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

PR Template: Add checklist item for changelog #5176

Merged
merged 5 commits into from Mar 24, 2023

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Mar 10, 2023

This PR adds a checklist item to update the changelogs on site to our PR template. Closes #5175.

@mejiaj mejiaj requested a review from thisisdano March 10, 2023 16:28
@mejiaj mejiaj linked an issue Mar 10, 2023 that may be closed by this pull request
@mejiaj mejiaj self-assigned this Mar 10, 2023
- Describe what makes a breaking change
- Add section for related PRs
- Remove  checkbox item related to Changelog
Comment on lines 42 to 48
<!--
Breaking changes include:
- Changes to the JavaScript API
- Changes to markup or content in our components
- Significant changes to the display of a component
If applicable, explain what actions are required for the user to remediate the break.
-->
Copy link
Member

Choose a reason for hiding this comment

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

In general, I don't think we should add a Changelog requirement to our PR template, but assure that all the information is there to add one if we need to as part of the release process. To that end, I've added a section on what makes a breaking change...

Comment on lines 59 to 66
## Related pull requests

_Indicate if there are other pull requests necessary to complete this issue_

<!--
Some changes to the USWDS codebase require a change to the documentation site, and need a pull request in the uswds-site repo. This could include new or updated component documentation, new or updated settings documentation, or changelog entries. Add links to any related PRs in this section. If this change requires an update to the uswds-site repo, but that PR does yet exist, just make sure to note that here.
-->

Copy link
Member

Choose a reason for hiding this comment

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

...and I added a new section to either link a related PR (or PRs) for documentation, etc, or at the very least, indicate that it's necessary but not yet done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great notes, thanks for making these changes!

Comment on lines 63 to 65
<!--
Some changes to the USWDS codebase require a change to the documentation site, and need a pull request in the uswds-site repo. This could include new or updated component documentation, new or updated settings documentation, or changelog entries. Add links to any related PRs in this section. If this change requires an update to the uswds-site repo, but that PR does yet exist, just make sure to note that here.
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

These new additions are great!
One small thing: it would be good to introduce line breaks in these comments to make it more readable in GitHub source. One possibility:

Suggested change
<!--
Some changes to the USWDS codebase require a change to the documentation site, and need a pull request in the uswds-site repo. This could include new or updated component documentation, new or updated settings documentation, or changelog entries. Add links to any related PRs in this section. If this change requires an update to the uswds-site repo, but that PR does yet exist, just make sure to note that here.
-->
<!--
Some changes to the USWDS codebase require a change to the documentation site,
and need a pull request in the uswds-site repo.
This could include:
1. New or updated component documentation
2. New or updated settings documentation, or
3. Changelog entries.
Add links to any related PRs in this section.
If this change requires an update to the uswds-site repo, but that PR does yet exist,
just make sure to note that here.
-->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this change in 0d5d451. I've also:

  • Changed it to an unordered list to match previous lists
  • Added additional new lines for clarity
  • Moved text to new line after 80 characters
  • Condensed last paragraph

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Great additions!
I added a missing period in 1ab8591 and made some suggestions for additional line breaks in this comment. Let me know if you have questions.

@mejiaj mejiaj requested a review from amyleadem March 21, 2023 13:42
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Made a few more suggestions for some small changes. Other than that, looks good to me!

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

This looks good to me! I like that we define breaking changes. Hopefully, users will flag documentation updates when necessary.

Left one suggestion to link to the USWDS-Site repo where it's mentioned but happy about this section either way!

- Fixed spacing
- Added link to uswds-site
- Add missing word

Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Co-authored-by: mahoneycm <charlie.mahoney@bixal.com>
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looks good!

@mejiaj mejiaj merged commit 62552ea into develop Mar 24, 2023
3 checks passed
@mejiaj mejiaj deleted the jm-pr-template-changelogs branch March 24, 2023 18:41
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.

Add changelog to PR checklist
4 participants