Active search result sticks to bottom and top of page #330
Conversation
|
Going down is looking good! I think we should make it behave the same way going up: when the user hits the up arrow when the last item is selected, what’s visible in the scroll area should stay the same while the selection moves up. See the gif below for the unwanted behavior: |
chasenlehara
changed the title
|
@chasenlehara this PR has been updated to address your last comment. |
…into 276-active-result-sticks-to-bottom
imaustink
changed the title
static/search.js
Outdated
| if(isMovingDown){ | ||
| var resultsContainerHeight = this.$resultsContainer.height(); | ||
|
|
||
| var isBellow = activeResultPosTop > resultsContainerHeight; |
imaustink
requested review from
andrejewski,
bgando and
chasenlehara
and removed request for
chasenlehara
There was a problem hiding this comment.
Overall looks good. Check out my comments for a couple small things. A medium-sized thing to consider is that this method has 2 jobs: toggling the class and adjusting the scroll. There isn't a ton of code so maybe it doesn't matter that much, but personally I'd break them apart.
static/search.js
Outdated
| // sets property and adds class to active result | ||
| activateResult: function($result){ | ||
| // Get position top of last active element | ||
| var lastResultPosTop = parseInt(this.$activeResult && this.$activeResult.position().top || 0); |
There was a problem hiding this comment.
Make sure to always pass 10 as the second argument to parseInt (you can search for reasons why).
static/search.js
Outdated
| var isAbove = activeResultPosTop < 0; | ||
|
|
||
| if(isMovingDown){ | ||
| if(isBellow){ |
There was a problem hiding this comment.
isMovingDown && isBelow could be on one line
static/search.js
Outdated
| this.resetScrollToBottom(activeResultPosTop, resultsContainerHeight); | ||
| } | ||
| }else{ | ||
| if(isAbove){ |
static/search.js
Outdated
|
|
||
| if(isMovingDown){ | ||
| if(isBellow){ | ||
| this.resetScrollToBottom(activeResultPosTop, resultsContainerHeight); |
| var resultsContainerHeight = this.$resultsContainer.height(); | ||
|
|
||
| this.$resultsContainer.scrollTop(this.$activeResult.position().top - activeResultOffset); | ||
| var isMovingDown = lastResultPosTop < this.$activeResult.position().top; |
There was a problem hiding this comment.
Add a comment here to clarify that you are talking about the arrow key direction
static/search.js
Outdated
|
|
||
| this.$resultsContainer.scrollTop(this.$activeResult.position().top - activeResultOffset); | ||
| var isMovingDown = lastResultPosTop < this.$activeResult.position().top; | ||
| var isBellow = activeResultPosTop > resultsContainerHeight; |
|
@bgando I agree with your reasoning behind splitting up |
|
@chasenlehara do you want it to stop when it reached the bottom or top? Currently, it restarts from the other end. |
|
I personally don’t like that it goes back to the top when you reach the bottom, but I’m willing to see how people react to it in practice. |
chasenlehara
left a comment
chasenlehara
left a comment
There was a problem hiding this comment.
Looks great! I added a comment for one minor thing.
static/search.js
Outdated
| // sets property and adds class to active result | ||
| activateResult: function($result){ | ||
| // Get position top of last active element | ||
| var lastResultPosTop = parseInt(this.$activeResult && this.$activeResult.position().top || 0); |
There was a problem hiding this comment.
Make sure to always pass 10 as the second argument to parseInt (you can search for reasons why).
Uh oh!
There was an error while loading. Please reload this page.