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.

Sign up for GitHub

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

Simplified pull request template #21900

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Simplified pull request template #21900

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50f6174
Simplified pull request template
vkjr Jan 8, 2025
b8111db
review notes
vkjr Jan 9, 2025
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
50 changes: 16 additions & 34 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,26 @@
[comment]: # (Please replace ... with your information. Remove < and >)
[comment]: # (To auto-close issue on merge, please insert the related issue number after # i.e fixes #566)

If you submit PR for issue with bounty then write here Fixes #NN where NN is issue number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have bounties now

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have means of compensating work from external contribution. This has been done during 2024 and there are at least two developers that I know interested in that.

But I think we should keep the removal as you did. This is unnecessary noise.

Copy link
Contributor

@ulisesmac ulisesmac Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ilmotta is there a public way to apply as an external contributor? I know a few that'd be interested on it

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's talk in private about this @ulisesmac, there are some important details first. If anybody is interested to understand this point, please reach out to me.


*otherwise*

fixes #...

### Summary

## Summary
[comment]: # (Summarise the problem and how the pull request solves it)
...

<!-- (Optional, remove if no changes to documentation) -->
Documentation change PR (review please): https://github.com/status-im/status.im/pull/xxx
Copy link
Contributor Author

@vkjr vkjr Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We do not change documentation frequently


### Review notes
<!-- (Optional. Specify if something in particular should be looked at, or ignored, during review) -->
## Review notes
[comment]: # (Optional. Specify if something in particular should be looked at, or ignored, during review)

### Testing notes
<!-- (Optional) -->
## Testing notes
[comment]: # (Optional)

#### Platforms
<!-- (Optional. Specify which platforms should be tested) -->
### Platforms
[comment]: # (Optional. Specify which platforms should be tested)

- Android
- iOS
- macOS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

desktop platforms aren't actual in status-mobile repo

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is related to changes in dev exp.

But yeah, they aren't common, let's remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ulisesmac, I think that was added back in times when it was possible to build desktop version of status-mobile using react-native-desktop-qt. That was long ago)

- Linux
- Windows

#### Areas that maybe impacted
<!-- (Optional. Specify if some specific areas has to be tested, for example 1-1 chats) -->
### Areas that may be impacted
[comment]: # (Optional. Specify if some specific areas need to be tested, such as 1-1 chats)

##### Functional
#### Functional

- 1-1 chats
- public chats
Expand All @@ -48,32 +35,27 @@ Documentation change PR (review please): https://github.com/status-im/status.im/
- fleet
- bootnodes

##### Non-functional
#### Non-functional

- battery performance
- CPU performance / speed of the app
- network consumption

### Steps to test
<!-- (Specify exact steps to test if there are such) -->
## Steps to test
[comment]: # (Specify exact steps to test if there are such)

- Open Status
- ...
- Step 3, etc.

<!-- (PRs will only be accepted if squashed into single commit.) -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is enforced by github machinery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this has confused a bunch of people over time. The only thing I'm not sure is that some devs may still prefer to merge via command line, in which case I'm not sure GitHub will reject if the commits aren't squashed. In status-go GitHub happily accepts multiple commits because I've seen this happen a few times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilmotta, well, if developer going to use command line, pull request comment won't stop it from happening.


### Before and after screenshots comparison
Copy link
Contributor Author

@vkjr vkjr Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Never seen this section used. Usually screenshots are attached in summary

Copy link
Contributor

Choose a reason for hiding this comment

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

This was used a couple times, for example I'm pretty sure I've seen in PRs for quo components. But fair enough to remove 👍🏼 I have a macro to generate an HTML table for before/after screenshots. At one point in Status I used this macro a lot, but never this part from the template.


| Figma (if available) | iOS (if available) | Android (if available)
| --- | --- | --- |
| Please embed Image/Video here of the before and after. | Please embed Image/Video here of the before and after. | Please embed Image/Video here of the before and after. |
[comment]: # (Can be ready or wip)
status: ready

status: ready <!-- Can be ready or wip -->

<!-- Uncomment this section for status-go upgrade/dogfooding pull requests

- Specify potentially impacted user flows in _Areas that maybe impacted*.
- Specify potentially impacted user flows in _Areas that may be impacted*.
- Ensure that _Steps to test_ is filled in.

### Risk
Expand Down