Skip to content
This repository has been archived by the owner on Sep 29, 2022. It is now read-only.

Click not handled after you return to a page #18

Closed
poshaughnessy opened this issue Jan 20, 2015 · 9 comments
Closed

Click not handled after you return to a page #18

poshaughnessy opened this issue Jan 20, 2015 · 9 comments
Assignees
Milestone

Comments

@poshaughnessy
Copy link

Thanks for this library! It's working well for me, except...

handleClick is assigned as an event listener to the DOM node in componentDidMount. However, if you navigate to another 'page' in your Single Page App, and then go back to your previous 'page', componentDidMount doesn't fire again for that first page, and the original event listener is lost? (That's what happens for me anyway - testing with Chrome v39). This means that when you now click another link on that first page, it won't be caught - it will result in a full page request.

How about also adding the event listener in componentDidUpdate? I tried it and it fixed the problem for me. (I was worried it might keep adding more and more event listeners, but it looks like addEventListener shouldn't allow duplicates*).

If this sounds good, it's just a case of adding:

componentDidUpdate: function() {
    this.getDOMNode().addEventListener('click', this.handleClick, false);
},

(I could issue a pull request if you like).

*Or an extra cautious way could be to do removeEventListener just before the addEventListener?

@larrymyers
Copy link
Owner

Can you get me some more details so I can reproduce?

  1. Are you navigating to a url not handled by the router?
  2. Is the componentWillUnmount lifecycle method not being called on the component you're adding the RouterMixin to?

If the DOM tree isn't changing, then the event listener shouldn't be lost.

@poshaughnessy
Copy link
Author

Ah, that's good to know - perhaps I'm inadvertently doing something a bit strange then!

Let me see if I can reduce it down to a test case that I can share, then. I'll post an update here soon.

@larrymyers
Copy link
Owner

Any updates on what the issue was?

@poshaughnessy
Copy link
Author

Sorry for the delay, Larry. Just trying to build a little test case now. Hope to post it here tonight or tomorrow.

@poshaughnessy
Copy link
Author

OK, here is a quick test case: https://github.com/poshaughnessy/react-mini-router-test

NB. I used a standalone version of your library, that I made using browserify on the command line, because:

  • I didn't want to introduce a CommonJS compile step, for this simple test case
  • I tried installing it with bower, but it errored with: ENOTFOUND Package juliangruber/isarray=juliangruber/isarray not found?

Instructions / steps to reproduce the problem (also included in the README):

  • npm install
  • node server
  • Fire up localhost:3000 in the browser
  • (Bring up the Network tab in the debugging tools so you can see when a full page request is made)
  • Click Page one - this will load without a full page request
  • Click Page two - this now causes a full page request

Tested in Chrome v40 and Firefox v35.

Hope this helps!

@larrymyers larrymyers self-assigned this Jan 27, 2015
@larrymyers larrymyers added this to the v1.1 milestone Jan 27, 2015
@larrymyers
Copy link
Owner

@poshaughnessy Thanks! That will be extremely helpful in debugging. I've attached this issue to the 1.1 milestone, so expect to see it resolved this month.

@larrymyers larrymyers modified the milestones: 1.2, v1.1 Feb 1, 2015
poshaughnessy pushed a commit to poshaughnessy/react-mini-router that referenced this issue May 5, 2015
@holyjak
Copy link

holyjak commented Jun 1, 2015

There is actually a very simple workaround, at least for us it worked well - wrap the component produced by the route function (that is different for each route) into a <div> - which will get the listener attached to itself, does not change, and thus doesn't lose the listeners.
I.e. instead of

render: function() {
        return this.renderCurrentRoute();
    },

do this

render: function() {
        return (<div>{this.renderCurrentRoute()}</div>);
    },

in your root component with the RouterMixin.

@rapind
Copy link

rapind commented Aug 22, 2015

I ran into this issue today. @jakubholynet 's solution worked.

@larrymyers larrymyers modified the milestones: 2.1, 1.2 Nov 18, 2015
@ghost
Copy link

ghost commented Oct 1, 2016

Still running into this, @jakubholynet's solution helped me out! Thank you. @larrymyers it'd be awesome if this was noted on the README if the code fix is a little ways off!

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

No branches or pull requests

4 participants