search

lsirivong / gatsby-plugin-modal-routing

A gatsby plugin for routable modals
44 stars 30 forks source link

Needs to merge PR to fix location accessing #51

Closed shavidzet closed 4 years ago

shavidzet commented 4 years ago

https://github.com/lsirivong/gatsby-plugin-modal-routing/pull/50

lsirivong commented 4 years ago

Hi @shavidzet, I commented on that PR as well, but could you help me understand what the issue is that this PR fixes? As far as I can tell, it is just removing a reference to an unused variable. I'm not able to see how that addresses an issue.

shavidzet commented 4 years ago

@lsirivong Hi,

As I debugged

const shouldUpdateScroll = ({
  prevRouterProps: { location: prevLocation },
  routerProps: { location }
}) => {

prevRouterProps is not object and when you try to destruct it, it throws runtime exception, it happens after upgrading gatsby to latest version.

shavidzet commented 4 years ago

That's why I forked your repo and made same change (https://github.com/shavidzet/gatsby-plugin-modal-routing/commit/cf8ded3e3bb2efd0a7250b12f928fa7de1357d29#diff-7e8a5eb2ef2dc38a4825df48a18d4fc9L4) recently was made in #50

machineghost commented 4 years ago

I assume this fixes the following?

shouldUpdateScroll.js:11 Uncaught TypeError: Cannot read property 'location' of undefined at Object.shouldUpdateScroll (shouldUpdateScroll.js:11) at api-runner-browser.js:35 at Array.map (<anonymous>) at ./.cache/api-runner-browser.js.exports.apiRunner (api-runner-browser.js:22) at ScrollHandler.shouldUpdateScroll (navigation.js:119) at ScrollHandler._this.shouldUpdateScroll (scroll-handler.js:67) at ScrollHandler._this.windowScroll (scroll-handler.js:46) at ScrollHandler.componentDidMount (scroll-handler.js:87) at ScrollHandler.componentDidMount (react-hot-loader.development.js:704) at commitLifeCycles (react-dom.development.js:19815) at commitLayoutEffects (react-dom.development.js:22804) at HTMLUnknownElement.callCallback (react-dom.development.js:189) at Object.invokeGuardedCallbackDev (react-dom.development.js:238) at invokeGuardedCallback (react-dom.development.js:293) at commitRootImpl (react-dom.development.js:22542) at unstable_runWithPriority (scheduler.development.js:653) at runWithPriority$1 (react-dom.development.js:11040) at commitRoot (react-dom.development.js:22382) at finishSyncRender (react-dom.development.js:21808) at performSyncWorkOnRoot (react-dom.development.js:21794) at scheduleUpdateOnFiber (react-dom.development.js:21189) at updateContainer (react-dom.development.js:24374) at react-dom.development.js:24759 at unbatchedUpdates (react-dom.development.js:21904) at legacyRenderSubtreeIntoContainer (react-dom.development.js:24758) at render (react-dom.development.js:24841) at app.js:94

If so @lsirivong please merge! I just tried to use the library today and ran into this.

shavidzet commented 4 years ago

@machineghost I've forked this repo, fixed current bug and shipped as npm pack, so you can use https://www.npmjs.com/package/gatsby-plugin-modal-routing-v2 for temporary purposes, after #50 will be done I will be back to this repo

machineghost commented 4 years ago

I tried using that package, and it does make things work, but when I click my Links (either Gatsby ones with state={{modal: true}}, or ones imported from your package with asModal) ... they just act as ordinary links, and take me to my modal as a separate page.

Since I've never used the library before I'm not sure if this is a separate issue or a side effect of your fix.

P.S. My dialog page/file is a carbon copy of the example one on the GitHub documentation (just with a different name).

chardmd commented 4 years ago

Screenshot 2020-06-28 at 3 06 11 PM

shavidzet commented 4 years ago

I tried using that package, and it does make things work, but when I click my Links (either Gatsby ones with state={{modal: true}}, or ones imported from your package with asModal) ... they just act as ordinary links, and take me to my modal as a separate page.

Since I've never used the library before I'm not sure if this is a separate issue or a side effect of your fix.

P.S. My dialog page/file is a carbon copy of the example one on the GitHub documentation (just with a different name).

Probably it's related with my fix, because I've deleted prevRouteProps usage in https://github.com/shavidzet/gatsby-plugin-modal-routing/commit/cf8ded3e3bb2efd0a7250b12f928fa7de1357d29#diff-7e8a5eb2ef2dc38a4825df48a18d4fc9L4 and probably prevRouteProps is necessary to stay on same page after opening modal.

Please provide your code on https://codesandbox.io/

lsirivong commented 4 years ago

Hi folks, this has shipped in 1.2.0.