My PR to firefox-ios

Firefox for iOS is the mobile version of the popular Mozilla Firefox web browser designed specifically for Apple's iOS operating system. I'm very glad to have contributed to firefox-ios. I would like to share my PR process. Hope it's helpful to you.

Issue

The issue was opened by data-sync-user. This issue was about the refactor request that replace previous button with the new designed PrimaryRoundedButton. In this issue, we can also see a reference PR by lmarceau from Mozilla. That was really helpful for me to start.

Refactor process

Prepare working environment

Walk through the official Building the code process. I want to mention two things of the steps:

sh ./bootstrap.sh

I met the following error when running this command on my M1 macbook air.

xcrun: error: unable to lookup item 'PlatformPath' from command line tools installation
xcrun: error: unable to lookup item 'PlatformPath' in SDK '/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk'

I fixed this by: (Solution provided by l'L'l )

$ sudo xcode-select -switch /Applications/Xcode.app/Contents/Developer

Make sure to select the Fennec scheme in Xcode.

This means setting scheme from where the blue box shown in below screenshot:

Others should be all provided in the repo's README.md.

Learn from the reference PR and Refactor

I learned from the reference PR how to refactor. It was like updating the definition then remove unused variables. For example:

Before:

   private lazy var mainButton: ResizableButton = .build { button in
        button.contentHorizontalAlignment = .center
        button.buttonEdgeSpacing = 0
        button.layer.cornerRadius = UX.mainButtonCornerRadius
        button.contentEdgeInsets = UX.mainButtonInsets
        button.addTarget(self, action: #selector(self.didTapMainButton), for: .touchUpInside)
   }

After:

   private lazy var mainButton: PrimaryRoundedButton = .build { button in
        button.addTarget(self, action: #selector(self.didTapMainButton), for: .touchUpInside)
   }

You can see now we don't need the contentHorizontalAlignment and buttonEdgeSpacing settings. This is because PrimaryRoundedButton has already defined.

Conflicts and Rebase

When the review was going on, I got a notation that informed my pr has conflicts when rebasing.

This means the main branch has updated and my PR can't merge to the main branch due to the conflicts. Here is how I did to resolve:

1. Sync fork

On the forked repo, click the Sync fork button and let Github do for you to keep the forked repo's main branch up to date with mozilla-mobile/firefox-ios:main

2. Update local repository

git checkout main
git pull origin main

3. switch to the branch where have made the changes, here mine is issue-16864

git checkout issue-16864

4. Rebase

git rebase main

Here we need to resolve conflicts manually, I used VS Code located my modified files and it could let me select which code to use.

5. Check status

After resolving conflicts, I can see the following info from git status

On branch issue-16864 Your branch and 'origin/issue-16864' have diverged, and have 26 and 1 different commits each, respectively. (use "git pull" to merge the remote branch into yours)
nothing to commit, working tree clean

The git status message indicates that my local branch and the remote branch have diverged due to the rebase, but there are no uncommitted changes in my working directory. This is expected after a successful rebase.

Note that although it shows

use "git pull" to merge the remote branch into yours

We don't need to do that.

Also note that on the VS Code SOURCE CONTROL Tab, it shows me a button with Sync Changes 1 down and 26 UP. This is a shortcut that performs both a git pull and a git push operation. The numbers (1 down, 26 up) represent the number of commits that are different between my local branch and the remote branch. "1 down" means there is 1 commit on the remote branch that I don't have locally, and "26 up" means there are 26 commits locally that are not on the remote branch.

However, in my case, I should not use the "Sync Changes" button. This is because I've performed a git rebase, which has rewritten the commit history of my local branch. If I were to sync now, it would try to merge the divergent histories of the local and remote branches, which is not what I want.

6. Force Push

git push origin issue-16864 --force

What we do after rebase is force push the my local branch issue-16864 to the remote repository to overwrite the remote branch with my local branch.

A little More

After force push, I can see in the PR page, there is a from commit-a to commit-b and a compare link

Follow the link, I can see many changes compare to the base: main

The "compare" link on GitHub shows the differences between the two points in history I'm comparing. This means that the comparison will include all changes that happened on the main branch since the point where my branch originally diverged from it. On the other hand, the "Files changed" tab in the pull request only shows the changes I made in my branch.

If my pull request is accepted and merged, only the changes I made in my branch (the ones shown in the "Files changed" tab of the pull request) will be merged into the main branch.

The other changes that I see when I click the "compare" link are changes that have already been made in the main branch. These changes are already in the main branch, so they won't be affected by my pull request.

Reflection

This contributing process has allowed me to learn more about the Git workflow and the concepts of controllers and view models, which has been a valuable learning experience. I even chatted with lmarceau from the Element channel #fx-ios and appreciate the help I've received from the community. I'm excited to continue my journey and further contribute to this project. Looking forward to seeing you there!