Mistakes to Avoid Before Submitting Your Pull Request
Oliver Pham
Posted on October 27, 2021
After browsing through a lot of beginner-friendly issues, one issue in the Element repo caught my attention. In brief, some text in languages other than English overflows a button in the Home page. I notice I've made several mistakes while completing the PR. If you are a first time contributor like me, you may want to avoid:
- Skipping the contribution guidelines
- Pushing outdated code
First look
Here's a picture of the issue:
It seems that a small CSS fix (revealed below) should resolve the issue, so I asked the maintainers of the repo to work on it. Not until I cloned the repo did I realize the tricky part was building the project for development.
I thought it was just another React project, so I made a pro-gamer's move: skip any type of tutorial. As I result, I wasted 2 days poking around the wrong repo.
Why you should not skip contribution guide
Unlike any open source projects to which I've contributed, this project involves 2 other repos, matrix-react-sdk and matrix-js-sdk. As explained in the Development
guide in the element-web
repo, I need those 2 SDKs in order to build Element successfully for code contribution.
The idea of Element is to be a relatively lightweight "skin" of customisations on top of the underlying
matrix-react-sdk
.matrix-react-sdk
provides both the higher and lower level React components useful for building Matrix communication apps using React.
In other words, despite the issue being filed at the element-web
repo, the code I needed to change was actually underneath the matrix-react-sdk
repo. Such a plot twist, wasn't it? Lesson learned: reading the guidelines actually saves you more time.
After pulling the code from the right repo, the part of the code responsible for the bug could be easily identified. First, the CSS class of the buggy component can be looked up with Chrome DevTools. Then, a global search on Visual Studio Code (Shift + Cmd + F
on Mac, Shift + Ctrl + F
on Windows) can help you reveal its location if it exists in the codebase.
It was something like this:
.mx_HomePage_default_buttons {
margin: 60px auto 0;
width: fit-content;
.mx_AccessibleButton {
padding: 73px 8px 15px; // top: 20px top padding + 40px icon + 13px margin
width: 160px;
height: 132px;
...
}
}
Why you should update your branch
Once I had the fix, I immediately pushed my code up to my GitHub fork and made a PR. This resulted in my PR failing to pass the CI tests. I didn't change a lot of code, so what could have gone wrong?
One collaborator of the project pointed out to me something really useful.
Well, there were actually two useful things, but only one of them helped me pass the CI. There was another code reviewer who left a comment on how I could improve my code further by changing height
to min-height
.
Finally, my PR was completed and merged thanks to help from the members of the Element project.
The same page after my fix was applied:
How I fixed it:
.mx_HomePage_default_buttons {
display: flex;
...
.mx_AccessibleButton {
...
min-height: 132px;
...
}
}
It was my first accepted PR in such a large project with more than 500 contributors. Although it was just a small fix, I couldn't help feeling proud while looking at the result of my contribution and my name in the project's change log. Nevertheless, at the end of the day, the two most important things that I've learned are the mistakes I mentioned above.
Posted on October 27, 2021
Join Our Newsletter. No Spam, Only the good stuff.
Sign up to receive the latest update from our blog.