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

Navigating away while transitioning sets incorrect scroll position #13

Closed
MartinMuzatko opened this issue Mar 13, 2018 · 9 comments
Closed

Comments

@MartinMuzatko
Copy link

When navigating away from a popup (using the arrow keys) in the middle of an animation to scroll to the visible popup, there is no other scroll event happening to get the correct focus.

scrollmove

@kamranahmedse
Copy link
Owner

Hey @MartinMuzatko could you please test if it is fixed now?

@kamranahmedse kamranahmedse added the awaiting-response Awaiting response from the issue opener label Mar 14, 2018
@MartinMuzatko
Copy link
Author

Is it already deployed to the website? Then it is not working yet :)
I'll install it locally and test it.

@kamranahmedse
Copy link
Owner

It is deployed. yes please and maybe try to debug this section :)

@kamranahmedse kamranahmedse removed the awaiting-response Awaiting response from the issue opener label Mar 14, 2018
@moghya
Copy link
Contributor

moghya commented Mar 16, 2018

@kamranahmedse @MartinMuzatko I think this is happening because we are choosing (please check here) postion to scroll as middle on the basis of absoluteElementTop and window.pageYOffset. When we're navigating away our element is already very close to the bottom of window i.e window.pageYOffset. and this way our middle is also very close to window.pageYOffset and we're loosing the entire point of scrolling to the middle of element.

I'll try to fix this. Any inputs from your side ?

P.S
Another thing affecting is popoverElement.bringInView(); call done before highlightedElement.bringInView(); call. I tried to commenting popoverElement.bringInView(); and now elements were always in middle of the screen, just that sometimes popoverElement were not in view.

@moghya
Copy link
Contributor

moghya commented Mar 16, 2018

@kamranahmedse @MartinMuzatko I'm extremely sorry, all the things I talked above were my misunderstandings,

Actual problem is ** popoverElement.bringInView(); is never bringing popover in the view**

I did little a small change for debugging,

if (popoverElement && !popoverElement.isInView()) {
      popoverElement.bringInView();

      //added this if to check effect of bringInView() call
      if (!popoverElement.isInView()) {
          console.log('it was not in view and still not in view');
      }
}

every time when popover was not in view, bringInView() failed to bring it inView.

@moghya
Copy link
Contributor

moghya commented Mar 16, 2018

@kamranahmedse @MartinMuzatko surprisingly adding following code fixed it, though this is not a good way to fix it.

while (!this.isInView()) {
    this.bringInView();
}

@kamranahmedse
Copy link
Owner

Fixed in 62c2a38. Should be fine now.

@moghya
Copy link
Contributor

moghya commented Mar 17, 2018

@kamranahmedse I'm sorry but that still leaves bug present in here

@kamranahmedse
Copy link
Owner

@moghya no, there is not.

The issue was related to the scroll, previously we had smooth scroll so it did not immediately scroll to the highlighted element; and if user switches the active element during the scroll, the next scroll was ignored because of the previous scroll being in progress. I just changed it to be instant to make it immediately scroll to the element.

The one that you are referring to, it is already instant and has no issues.

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

Successfully merging a pull request may close this issue.

3 participants