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 PR description to add feature checks and possible screenshots/videos #2794

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

fpena
Copy link
Collaborator

@fpena fpena commented Oct 1, 2024

Please verify the following:

  • yarn test jest tests pass with new tests, if relevant
  • yarn lint eslint checks pass with new code, if relevant
  • yarn format:check prettier checks pass with new code, if relevant
  • README.md (or relevant documentation) has been updated with your changes

Describe your PR

This PR updates Ignite's PR template with:

  • Add a new check to enforce maintainers to test features
  • Add new section for before/after screenshots

@fpena fpena changed the title Update PR description to add feature checks and possible screenshots/videos [2786] Update PR description to add feature checks and possible screenshots/videos Oct 1, 2024
Copy link
Contributor

@lindboe lindboe left a comment

Choose a reason for hiding this comment

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

left some suggestions for edits, let me know what you think

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
## Please verify the following:

- [ ] `yarn test` **jest** tests pass with new tests, if relevant
- [ ] `yarn lint` **eslint** checks pass with new code, if relevant
- [ ] `yarn format:check` **prettier** checks pass with new code, if relevant
- [ ] `README.md` (or relevant documentation) has been updated with your changes
- [ ] Include issue number in title and description
Copy link
Contributor

@lindboe lindboe Oct 2, 2024

Choose a reason for hiding this comment

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

  1. I don't think we need it in both title and description, I think just one is fine (and I would vote for description, otherwise the auto-generated messages for squash commits will include both an issue number and PR number, not ideal)
  2. We say in description here, but added a field above that's separate from the description section. I think it's fine to just include instructions for the description in a comment like suggested below, and then we don't need this checkbox.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For 2, when I say description, I meant the first part of the template. I'm fine with the change you suggested above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I really like the issue number in the title. It makes it easier to search from the PR list, and it forces the creator to match with an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

A few thoughts:

  1. I don't know if we want to always require an issue for every PR. We tend to use issues mostly for bug reports or things we can't get to right away, and I'm not sure adding issues for each PR is beneficial. @frankcalise may have thoughts here?
  2. I do appreciate the searchability question. I've usually relied on backlinks from the description to navigate from issues themselves (which titles can't give us), but those can be harder to find on issues with a lot of activity.

On point 2, I've noticed GitHub now has this "Development" field on the right-hand column of issues and PRs to link PRs with issues so I went looking for more docs about that and what else we can use: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue.

It looks like we can use that sidebar, but it's maybe even more convenient to use the "Closes #number" syntax. I mostly care that we get links to the issue via the "Development" field for easy navigation between PR/issue.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I haven't noticed that field.

Ok so then, let's rely on that plus the comment added in this PR.
Should we update the comment from:

<!-- If this PR addresses an issue, link to it in description: "Addresses #333" -->

to

<!-- If this PR addresses an issue, link to it in description: "Closes #333" -->

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the Closes #xyz will label it under the "Development" field for you, I use this quite a bit. Sometimes the Development field makes you choose the repo (unsure why, as you're already there) and then find the issue name. Either work, but I know that syntax works - unsure about Addresses

As far as an issue for each PR, we don't always have them. Sometimes an idea pops in your head and you just go for it (or have discussed it with the team if it needs discussing). So for IR folks, I don't think it is as much of a requirement.

For outside contributions though, it would be nice to understand what the issue is that they are trying to solve, but that of course could be done with a good PR write up as well, not really a requirement for me although low gafo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree! We leave it as a suggestion if the issue exists. Changed the working from "Addresses" to "Closes".

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
@fpena fpena requested a review from lindboe October 2, 2024 21:25
@fpena fpena requested a review from lindboe October 2, 2024 22:51
Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

Thanks, left some feedback for helping resize images so we don't end up doing this work each PR 😆


## Screenshots (if applicable)

If you’re submitting code changes related to a visible feature, please include before-and-after screenshots or videos.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we include an example of HTML markup for resizing images that people could reference.

<!-- If GH's auto attachment previews too large, trying resizing it with: <img src="filename-from-github-upload" width="300" /> -->

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my experience and within a markdown table, Github resizes the image to fit the space. And works also when having multiple columns.

Copy link
Collaborator Author

@fpena fpena Oct 2, 2024

Choose a reason for hiding this comment

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

However, some people can delete the table and add the image directly, which could end up taking a lot of space.

Copy link
Contributor

Choose a reason for hiding this comment

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

for sure, maybe just including this as a comment might be useful to people who want quick access to how to do it. otherwise it can be ignored.

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
@fpena fpena requested a review from frankcalise October 4, 2024 14:20
@fpena fpena changed the title [2786] Update PR description to add feature checks and possible screenshots/videos Update PR description to add feature checks and possible screenshots/videos Oct 4, 2024
Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

Some optional things to consider but looks good to me! Thanks!

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
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.

3 participants