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.

Sign up for GitHub

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

Jump to bottom

Fixes #4 - allow other elements to scroll #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes #4 - allow other elements to scroll #37

wants to merge 2 commits into from

Conversation

ajmas
Copy link

@ajmas ajmas commented Feb 19, 2018

No description provided.

@ajmas ajmas changed the title Fix for issue #2 - allow other elements to scroll Fix for issue #4 - allow other elements to scroll Feb 19, 2018
@ajmas ajmas changed the title Fix for issue #4 - allow other elements to scroll Fixes #4 - allow other elements to scroll Feb 19, 2018
@ajmas ajmas mentioned this pull request Feb 19, 2018
Open
@heavybeard
Copy link

Can someone take care about this?

@ajmas
Copy link
Author

ajmas commented Apr 8, 2018

I’ll send an email to @gabergg and see if he is still maintaining the project.

@dbramwell
Copy link

Not sure if I'm doing something wrong, but I had a play with your code earlier and hit a problem. The "container" is set before the app has been mounted, meaning that if I set "container: document.getElementById('whatever')" it ends up setting the container to null. I've tried setting the container in an componentDidMount but by the time the component had mounted container was already set to window.

Might it work if we pass an id instead of an element as the container and then let the manager find the element once the app has loaded?

If I'm doing something obviously wrong please let me know.

@ajmas
Copy link
Author

ajmas commented Apr 11, 2018

When do you call configureAnchors?

@dbramwell
Copy link

dbramwell commented Apr 12, 2018 •
edited
Loading

I've tried two places, straight away at the top of the file and in componentDidMount. First option is too soon, the container doesn't exist yet. Second option is too late, container is already set to window.

As I mentioned, you could get around this by passing an ID to configureAnchors and then only finding the element in the addAnchor method.

A bigger issue that I later found is that jump.js (it is used here to provide the smooth scrolling effect) only works with the window object:
https://github.com/callmecavs/jump.js/blob/master/src/jump.js#L53

It seems to have been raised as an issue there, and people have created pull requests, but the owner doesn't seem to have addressed it:
callmecavs/jump.js#50

Someone in there suggests another library, but I don't know if @gabergg would be willing to switch to velocity.js?

@ajmas
Copy link
Author

ajmas commented Apr 12, 2018

If I or someone else has time, then maybe an example could be added? I don’t have answer for you at the moment.

@dbramwell dbramwell mentioned this pull request Apr 13, 2018
Open
@dbramwell
Copy link

Built on your changes, created a pull request and have an example built see example 5

I found that zenscroll allows us to scroll elements as well as the window, so I've switched from jump.js to that.

Would be interested to know what you think, can you spot anything I might have missed?

@dbramwell
Copy link

Also, don't suppose you got a response from @gabergg ?

@dfitz89
Copy link

dfitz89 commented May 10, 2018 •
edited
Loading

Any updates on this PR?...the example that dbramwell created is exactly what I need...thanks in advance.

@dbramwell
Copy link

If you want to use my code you can do so by changing the dependency in your package.json to:

"react-scrollable-anchor": "github:dbramwell/react-scrollable-anchor#release"

@gabergg doesn't seem to be maintaining this project at the moment, so I'm not sure when this will progress

@ajmas
Copy link
Author

ajmas commented May 10, 2018

@dbramwell I never heard back. You may want to make you version available via npm, maybe as react-scrollable-anchor-2?

@dbramwell
Copy link

I'll see if I get chance at some point. Bit of a crap outcome though. Someone asked in another pull for another maintainer to be added, hopefully that happens at some point. Would be nice if that happened as this thing has quite a following.

@dfitz89
Copy link

dfitz89 commented May 10, 2018 via email

@dbramwell
Copy link

Can you give me more info? What do the errors say? Any chance I can see the code?

The ScrollAnchors need to be inside the scrollable container.

@dfitz89
Copy link

dfitz89 commented May 10, 2018 via email

@dbramwell
Copy link

"Cannot read property 'addEventListener' of null" - this is the telling part. Means that config.container is set to null.

this.config.container = document.getElementById(this.config.containerId)

The fact that it's null means that there are no elements found by the id that you are passing into configureAnchors. Are you sure you are using the correct id of your scrollable container? Notice where 'scrolling-div' is set in example 5:
https://github.com/dbramwell/react-scrollable-anchor/blob/master/example/src/components/Example5.js

Sorry for the lack of formatting in this comment, writing on a phone

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dbramwell
Copy link

dbramwell commented May 11, 2018 •
edited
Loading

A couple of questions now.

  1. When you click a menu option in <SurveySidebarQuickNav />, does the hash in the url in the browser change? react-scrollable-anchor listens for hash changes, for example if you click the 'section 3' link on example page above, the url in the browser changes to 'https://dbramwell.github.io/react-scrollable-anchor/#section3'. The library sees this and then scrolls the component to <ScrollableAnchor id='section3' />

<div id="scrolling-div">
        <FormWrapper />
      </div>

You say FormWrapper loads the form which contains the anchors. The id passed to configureAnchors needs to point to the div that has the scroll bar. In your example, is it the div with the id 'scrolling-div', or is it infact whatever is created by FormWrapper, that has the scrollbar?

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dfitz89
Copy link

dfitz89 commented May 11, 2018 via email

@dbramwell
Copy link

Can you share the structure of the FormWrapper? If you have all the ids named correctly then the only thing I can think of that would cause this is the 'scrollable-div' not actually being scrollable. Are you sure the scrolling isn't happening on the FormWrapper itself? Or on something inside it?

I think it's unlikely that the ajax part is causing it. You can test that by disabling the ajax stuff and filling the FormWrapper with a static form and checking if you're still getting the same behaviour.

@dfitz89
Copy link

dfitz89 commented May 17, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ajmas @heavybeard @dbramwell @dfitz89