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

issue-1002-fixed #1201

Merged
merged 1 commit into from
Mar 16, 2017
Merged

issue-1002-fixed #1201

merged 1 commit into from
Mar 16, 2017

Conversation

yoshi2095
Copy link
Contributor

@yoshi2095 yoshi2095 commented Mar 6, 2017

Proposed changes in this pull request

Included the Search and Go-To-My-Location map controls in Project Overview and Map pages.

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

@shailysangwan
Copy link
Contributor

@yoshi2095 The checklist is meant for the reviewers to fill :)

@linzjax
Copy link
Contributor

linzjax commented Mar 8, 2017

Hi @yoshi2095, your solution works and your PR looks good to go, but I have one final request before we can merge your PR. Could you please rebase onto the latest changes in master? That will make merging your changes a lot easier for us.

You'll see that GitHub says "This branch is out-of-date with the base branch". That means that there have been changes in the main Cadasta/cadasta-platform repository that you don't have in your PR branch. That means that there's no way to know if your work is compatible with what's already been done. What you need to do in this case is to make sure that you've added our repo as an upstream remote for your fork (GitHub usually tells you to do this when you create a fork), fetch the latest master from there, and then update your PR branch to incorporate the latest changes. If you're working on a topic branch, you do this:

git checkout master  # This is your local master branch
git fetch upstream/master  # Get changes from Cadasta/cadasta-platform
git merge upstream/master  # This does a "fast-forward" merge
git checkout <topic-branch>  # Switch back to your topic branch
git rebase master  # Replay the changes you've made on top of the latest upstream master
git push --force  # Update your PR on GitHub

If there are conflicts between your work and what's been done in the main repo, after you do the rebase you'll get merge conflicts that you need to fix.

@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Mar 9, 2017

@linzjax done 👍

@linzjax
Copy link
Contributor

linzjax commented Mar 9, 2017

Hmmm... It looks like you're still 17 commits behind Cadasta's master branch. Is the cadasta repository set as your remote upstream? If you run:

git remote -v

it'll return something that looks like this:

origin	    [email protected]:<your-username>/cadasta-platform.git (fetch)
upstream    [email protected]:Cadasta/cadasta-platform.git (fetch)

If you don't see upstream [email protected]:Cadasta/cadasta-platform.git (fetch), you'll need to set your upstream branch:

git remote add upstream [email protected]:Cadasta/cadasta-platform.git

Then try running the rebase steps again.

@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Mar 9, 2017

@linzjax done 👍 . Earlier, I didn't have any public SSH keys set for my account. That's why it wasn't merging the commits. I made one, and its all good now. Thanks :)

@linzjax linzjax requested review from linzjax and clash99 March 9, 2017 18:13
@linzjax
Copy link
Contributor

linzjax commented Mar 9, 2017

@yoshi2095, thanks for your changes. This looks good. We will merge your contribution soon!

@yoshi2095
Copy link
Contributor Author

@linzjax Thank you :)

Copy link
Contributor

@clash99 clash99 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @yoshi2095!

@oliverroick oliverroick merged commit 29e02ef into Cadasta:master Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants