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

ability to jump between scrolling jump history #1082

Closed
wants to merge 9 commits into from
Closed

ability to jump between scrolling jump history #1082

wants to merge 9 commits into from

Conversation

fent
Copy link
Contributor

@fent fent commented Jun 25, 2014

Like in vim, Ctrl-O and Ctrl-I jump backward/forward respectively in jump history. "jump history" is basically any command that uses the scroller.jumpTo() function.

I've often needed to see something at the top/bottom of a tall page, but wanted to quickly go back to where I was. What I had to do was create a mark, jump, and then jump back to the mark. With this feature, jump history will already be recorded, having the user press less keys if they want to jump back.

Jump history is truncated if a user jumps back and then uses a scroller.jumpTo() command. History will also be updated if the user scrolls after they've jumped to a position and history, and then jumped again.

History is cleared if the activated element ever changes and is scrolled with a command. I wasn't sure how to keep history per element without editing the page structure. So it seemed like the best solution was to clear it.

@deiga
Copy link
Contributor

deiga commented Jun 25, 2014

This is a good feature and it looks solid, @fent please add some tests

@fent
Copy link
Contributor Author

fent commented Jul 1, 2014

Not sure why travis tests failed. Not familiar with the tests codebase.

@fent
Copy link
Contributor Author

fent commented Jul 20, 2014

hmm, locally tests pass

$ cake test
Running unit tests...
Pass (89/89)
Running DOM tests...
Pass (29/29)

not sure why travis build failed. also not sure where to begin adding the tests, it doesn't seem like many of the other commands have tests.

mrmr1993 added a commit to mrmr1993/vimium that referenced this pull request Oct 13, 2014
Conflicts:
	background_scripts/commands.coffee
@soyuka
Copy link

soyuka commented Oct 21, 2014

Should be mapped to g; too ;)

Also, you'll have a memory leak if you scroll a lot I haven't seen a maximum history size but only an [].push(). I'm sorry that I'm not familiar enough with coffeescript to help on this.

@smblott-github
Copy link
Collaborator

@fent. This looks promising. Could you fix the tests and consider @soyuka's comments, above.

Also, we need to think more about the bindings. <c-o> is used by chrome.

mrmr1993 added a commit to mrmr1993/vimium that referenced this pull request Nov 23, 2014
ability to jump between scrolling jump history

Conflicts:
	content_scripts/scroller.coffee
@fent
Copy link
Contributor Author

fent commented Feb 24, 2015

I've merged the latest changes. Max scrolling history added, and mapped g; to scrollBackward too.

Not sure what to do about having <c-o> mapped since chrome uses it. Maybe not have it mapped by default?

Also made it so previous animations are cancelled when Scroller.scrollTo() is called. This makes more sense as scrollTo() is used for absolute positions, running another animation while, would displace the final position.

@smblott-github
Copy link
Collaborator

We have this to some extent with #1714 (which is merged in #1716).

The additional functionality seems a bit niche; too niche to warrant new commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants