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

Fix #1148 -- clicking on search panel should go to the top of the page #1172

Closed
wants to merge 3 commits into from
Closed

Conversation

maitreyaverma
Copy link

@maitreyaverma maitreyaverma commented Mar 2, 2017

Fixes #1148

@CLAassistant
Copy link

CLAassistant commented Mar 2, 2017

CLA assistant check
All committers have signed the CLA.

@oliverroick
Copy link
Member

Hi @maitreyaverma, thank you for your contribution. I have suggestion: You've been committing work directly to the master branch of your repo, and you made the PR from master. That's not a good thing to do, because it can make it difficult to incorporate changes from the "upstream" Cadasta/cadasta-platform repository into your work, which you need to do to keep your PR branch up to date with other work that's going on in the main repo. The way to deal with this is to work on what people call a topic (or feature) branch. When you want to work on a feature, you check out your master branch, then do something like git checkout -b bugfix/#675, which makes a new branch with a mnemonic name, expecially for the work you're going to do, so that we can keep track of what's going on.

@maitreyaverma
Copy link
Author

@oliverroick Thank You for your suggestion. I will surely keep this in mind now onwards. I am planning to create a new branch for this issue in my repository and revert master branch to its original state. Is this approach correct?
Also, can you tell me if my solution is a correct way to solve this issue?

@linzjax
Copy link
Contributor

linzjax commented Mar 2, 2017

Hi @maitreyaverma, creating a new branch and reverting your master branch back to match the current cadasta branch is the correct approach. Once your PR is ready we can work through getting your branch up-to-date with our current master.

Your fix is on the right track, but unfortunately it won't quite work. Since the search results are in an isolated <div> that scrolls using overflow, technically the window will always be positioned at 0,0. So you'll need to find a way to get the <div> containing the search results page to scroll to the top.

@linzjax
Copy link
Contributor

linzjax commented Mar 8, 2017

Since you've opened a new pull request, I'm going to close this one.

@linzjax linzjax closed this Mar 8, 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.

4 participants