Add scroll-to-top when switching verticals from bottom nav buttons. #597
Conversation
| @render new_position | ||
| # `direction` can be 'seq_prev' or 'seq_next' | ||
| _change_sequential: (direction, event) => | ||
| console.log "_change_sequential #{direction}" |
There was a problem hiding this comment.
Remove? We didn't print to the console originally, and we're already making calls to our Logger and Segment.io's API.
|
I like the refactor. I'm getting the scrollup when I trigger a goto, and when I click a greyed-out arrow (i.e., can't go further left or right). Do we want this behavior? |
|
I'm also getting the scrollup when I use the top nav, even though it seems like it should only happen when the bottom nav is used. |
| sequence_id: @id | ||
| current_sequential: @position | ||
| target_sequential: new_position | ||
|
|
||
| # If the bottom nav is used, scroll to the top of the page on change. | ||
| if $(event.target).parent().parent().parent().hasClass 'sequence-bottom' |
There was a problem hiding this comment.
All those parent() calls look very fragile. Maybe use .closest('nav[class="sequence-bottom"]') (or whatever it may be) and check whether that's non-empty.
|
|
When any grey arrow is clicked, the page scrolls to the top. This is bad, but it doesn't call |
|
Could you add/update the tests in spec/sequence/display? |
|
@jkarni added test. |
| @@ -142,6 +142,9 @@ xdescribe 'Sequence', -> | |||
| it 'call render on the next sequence', -> | |||
| expect($('#seq_content').html()).toEqual 'Sample Problem' | |||
|
|
|||
| it 'scrolls to the top of the page', -> | |||
| expect($.fn.scrollTo).toHaveBeenCalled() | |||
|
|
|||
There was a problem hiding this comment.
This doesn't actually test the function's expected behavior. I'd say use $scrollTop instead, and check that it's 0 for the right element.
|
👍 if build passes. |
|
Clicking greyed-out nav arrows still scrolls us to the top. As @mlsteele mentioned, this may be worth hunting down in a separate PR. Otherwise, 👍 |
|
@jkarni, are we merging this in? |
|
@rlucioni I hadn't actually tried reproducing the issue you mentioned. Now I see that I can't - do you mean the bottom button, once you've gone as far in a direction as you can? |
|
I was referring to both top and bottom buttons. I'll try again to see if I can reproduce it. |
|
I can't reproduce the issue, so I say |
.Add scroll-to-top when switching verticals from bottom nav buttons.
Feature/victor/nested links
Hash update: Group Project V2
* vagupta16/fix-sneakpeek-enroll-nonexistent-course: Prevent sneakpeeks of nonexistent courses
…ucture-url Proversity/grades api structure url
…#597) Co-authored-by: Renovate Bot <bot@renovateapp.com>
Bug: When a student finishes a tall vertical and clicks the next arrow at the bottom of the page, the vertical advances but the students view remains at the bottom of the page.
This adds a scroll to top when the students clicks the nav buttons at the bottom of the page.
Also, a small refactor so that the buttons call the same method with an argument of
seq_nextorseq_prev.Reviewers:
@rlucioni
@jkarni