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

Add scrollOffset #225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add scrollOffset #225

wants to merge 1 commit into from

Conversation

tb
Copy link

@tb tb commented Jun 14, 2017

Extra parameter allows tuning react-scroll for react-md sidebar / toolbar. Fixes #143

@tb tb mentioned this pull request Jun 16, 2017
@fisshy
Copy link
Owner

fisshy commented Jun 16, 2017

Thanks for your PR.

Why can't you use Offset?

@tb
Copy link
Author

tb commented Jun 16, 2017

@fisshy I use offset, but in context of react-md top toolbar I need that new param to fix the scroll position generated by link click - in fact what I do is I cancel the offset like this:

nestedItems={
  sections.map(section => (
    <ListItem
      key={section.id}
      primaryText={section.headline}
      component={Scroll.Link}
      to={`section-${section.id}`}
      spy={true}
      offset={-64}
      scrollOffset={64}
    />
  ))
}

Without scrollOffset param I was not able to fix the link to scroll to proper position. And I cant skip offset because then the highlight on scroll does not work. So I either needed variable to skip offset on scrollTo helper or add scrollOffset which would allow me to adjust the scrollTo. SInce I found an issue and a PR doing scrollOffset (which for some strange reason incuded also some other changes) - I made new PR with only the scrollOffset :)

@maciejmyslinski
Copy link
Contributor

@fisshy any update on this one? I'd really love to see this merged!
@tb could you please resolve conflicts? (BTW hello from 🇵🇱dude!)

@laurens-sprakel
Copy link

I have the the same problem as this
#278
The spy offset and the scroll offset are not the same
Can we merge this or does anyone have different solution for this problem?

@ronny-rentner
Copy link

I have the same problem, no clue why this is not merged

@blimpmason
Copy link

+1

2 similar comments
@doxomo
Copy link

doxomo commented Apr 26, 2021

+1

@jakehobbs
Copy link

+1

@ongddree
Copy link

I have the same problem, please separate spy offset and scroll offset

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 this pull request may close these issues.

Allow to set an offset only for scrollspy
9 participants