|
| 1 | +# 🛠️ Working with Pull Requests (PR) |
| 2 | + |
| 3 | +## Code Review process |
| 4 | + |
| 5 | +When it comes to code review process, let's aim to make it efficient and convenient for both the reviewer and the author of the Pull Request. |
| 6 | + |
| 7 | +### Definition of Readiness |
| 8 | + |
| 9 | +A Pull Request is considered as ready for code review when: |
| 10 | + |
| 11 | +- All checkboxes in the pull request template are checked. |
| 12 | +- A review is requested from a team member who ideally has sufficient knowledge about the specific topic, feature, or area. Feel free to involve multiple individuals in the review process. |
| 13 | + |
| 14 | +### When Requesting a Review |
| 15 | + |
| 16 | +1. **Preserve the PR Template:** First of all, do not remove the pull request template unless it's necessary. Stick to the current template by filling in the required details. |
| 17 | + |
| 18 | +2. **Consider the Reviewer's Perspective:** When you submit your code for review, imagine yourself on the other side of the PR. |
| 19 | + |
| 20 | +- Is your code ready to be reviewed? |
| 21 | +- Are the needed details / clarifications provided in the description? |
| 22 | +- Is the needed person's review requested? |
| 23 | + |
| 24 | +3. **Keep Discussions within GitHub:** Prefer keeping the context inside the PR. E.g. minimize discussions around the code and the solution outside of GitHub. |
| 25 | + |
| 26 | +Below you can find recommendations / requirements for the PR organization. These are must-to-follow rules and is applicable for every PR with a few exceptions. |
| 27 | + |
| 28 | +### When Reviewing a PR |
| 29 | + |
| 30 | +Let's not talk about obvious things like being polite, giving constructive feedback etc. As a reviewer: |
| 31 | + |
| 32 | +- Ensure that the PR is ready to be reviewed before starting reviewing. Read the description to understand whether all conditions are met and review the code. Otherwise, request the author to update the missing details. |
| 33 | +- If a code is unclear and makes you confused, consider requesting documenting unclear parts of the code. Either in the code itself or maybe in a dedicated playbook. It won't hurt to put links to Confluence / other PRs right in the code to make sure that anyone will be able to recollect what happened in the code. Well... In an ideal world. |
| 34 | + Remember: it's very easy to lose context or understanding of something that is not 100% obvious. |
| 35 | +- When requesting a change, either generally or on specific lines, double check if your request is clear enough for another person to understand. Consider using [suggestions](https://stackoverflow.com/questions/60311158/how-can-i-suggest-multiple-lines-be-changed-in-markdown), attaching screenshots or links to make requests clearer. |
| 36 | + |
| 37 | +## Branch name |
| 38 | + |
| 39 | +The branch name must start with the conventional type (`feat` / `fix` etc), have the ticket number and the ticket name (in lower case) in it. |
| 40 | + |
| 41 | +Example: a ticket you resolve is named `Introduce new kind of something` and the number is ABC-123. The branch name for a PR from the previous example must either be `feat/ABC-123-introduce-new-kind-of-something` or just `feat/ABC-123`. |
| 42 | + |
| 43 | +## Commits |
| 44 | + |
| 45 | +We are using conventional commits: https://www.conventionalcommits.org/en/v1.0.0/#summary |
| 46 | +Please make sure your commits match the rules. |
| 47 | + |
| 48 | +## Testing your changes first |
| 49 | + |
| 50 | +Kindly test all the changes you've made in all possible scenarios: |
| 51 | + |
| 52 | +- Different device sizes |
| 53 | +- Different platforms |
| 54 | +- Different connection states |
| 55 | +- Etc. |
| 56 | + |
| 57 | +Remember that some features work differently on different platforms, devices etc. For example, the `TextInputType.none` does not work on iOS and may lead to a crash / non-openable keyboard. |
| 58 | + |
| 59 | +## Title naming convention (pay attention ⚠️) |
| 60 | + |
| 61 | +### Why? |
| 62 | + |
| 63 | +When we merge a PR into develop (or any other branch), we do `squash merging`. It's when all the commits of another branch are squashed into one commit, with the name usually set to the PR title. |
| 64 | + |
| 65 | +_It is important to follow this naming convention as we have a script that parses the merged PR titles into a changelog when we release a new version._ |
| 66 | + |
| 67 | +### Title schema & rules |
| 68 | + |
| 69 | +Here's a schema of the PR title: `{type}({tickets}): title`, where: |
| 70 | + |
| 71 | +- `type` is the type of the conventional commit. For example, a new feature must be `feat`, while a bug fix must either be `bug` or `fix`. |
| 72 | +- `tickets` (surrounded by `()`!) is/are the number(s) of the ticket(s) this PR resolves. Can either be one (`ABC-123`), or multiple, separated by comma (`ABC-123,ABC-322,ABC-100`). |
| 73 | + If a PR doesn't resolve any ticket (for example, it's a minor enhancement or a documentation update) it is okay to leave the PR title without the `{tickets}` part. Don't forget to remove the `()` brackets then. |
| 74 | +- `title` usually is the ticket title. If the ticket resolves multiple tickets, use some common name, like `Login updates`. If the ticket doesn't resolve any tickets choose a title that describes the changes best. |
| 75 | + |
| 76 | +Example: a ticket you resolve is named `Introduce new kind of something` and the number is ABC-123. The PR title must be: `feat(ABC-123): Introduce new kind of something`. |
| 77 | + |
| 78 | +## Description |
| 79 | + |
| 80 | +To make it simpler for those checking your work, please tell them about the changes you made and why you made them. It's essential to give a quick explanation because the reviewer might be dealing with a different project or a different scope and might not know much about your work. |
| 81 | + |
| 82 | +If there is a ticket associated with your work, please include a link to it. |
| 83 | + |
| 84 | +### Steps to test |
| 85 | + |
| 86 | +If your works has complex or unclear steps to test, kindly provide those. Something like this would work: |
| 87 | + |
| 88 | +1. Sign in as ... |
| 89 | +2. Go to ... |
| 90 | +3. Do ... |
| 91 | +4. Expect ... |
| 92 | + |
| 93 | +If it doesn't require any specific steps to test skip this part. |
| 94 | + |
| 95 | +### Showcase: videos and screenshots |
| 96 | + |
| 97 | +Adding videos and screenshots to your work helps reviewers understand what you're doing faster. It lets them see how things are going without having to open the app, catching any issues early on. This way, reviewers only start looking at the code when everything checks out in the videos or pictures. It's a straightforward way to make the review process quicker and easier. Keep in mind that the reviewer might be working in a very different context right now, so any information you provide will be useful. |
| 98 | + |
| 99 | +In case some changes were requested, kindly attach a new video / screenshot, if required. |
| 100 | + |
| 101 | +#### Why? |
| 102 | + |
| 103 | +Switching to another branch, rerunning (possibly) code generation and launching / relaunching the app might be time consuming. If the change is minor, it is worth to provide the reviewer a visual representation of the change, so they won't even need to launch the app on their side to test it out. |
| 104 | + |
| 105 | +### Add Screenshots in a better way |
| 106 | + |
| 107 | +If you just drag & drop or upload a screenshot to Github, it will be formatted like this: |
| 108 | + |
| 109 | +```markdown |
| 110 | + |
| 111 | +``` |
| 112 | + |
| 113 | +Which will make the image expand to the max available width. Instead, we should add images like this: |
| 114 | + |
| 115 | +```markdown |
| 116 | +# Width is approximate but usually works just fine. |
| 117 | + |
| 118 | +<img width=265 src="YOUR_ASSET_URL"/> |
| 119 | +``` |
| 120 | + |
| 121 | +> 💡 Tip of the day! |
| 122 | +> |
| 123 | +> Consider adding an `img` [text replacement](https://support.apple.com/en-gb/guide/mac-help/mh35735/mac) to your MacOS settings! |
| 124 | +
|
| 125 | +Notice the difference between these two: |
| 126 | + |
| 127 | +#### First formatting |
| 128 | + |
| 129 | +Images are very large and cannot fit the normal screen size. In order to view a screenshot, you need to open it in a separate tab or zoom out the screen (just like it's done on the screenshot) |
| 130 | + |
| 131 | +<img width=265 src="../_pictures/ScreenshotsBadExample.png"/> |
| 132 | +<p><em>Figure: Bad example of adding the screenshots</em></p> |
| 133 | + |
| 134 | +#### Second formatting |
| 135 | + |
| 136 | +Images are not large, even multiple images can be placed in one row next to each other (normal browser zoom). |
| 137 | + |
| 138 | +<img width=400 src="../_pictures/ScreenshotsGoodExample.png"/> |
| 139 | +<p><em>Figure: Good example of adding the screenshots</em></p> |
0 commit comments