search

lsirivong / gatsby-plugin-modal-routing

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

Attempt to enable custom modal components #54

Open machineghost opened 4 years ago

machineghost commented 4 years ago

Fix for: https://github.com/lsirivong/gatsby-plugin-modal-routing/issues/52

machineghost commented 4 years ago

Note: to understand the code I did a bit of refactoring, and it was easier to leave the refactorings in with my fix. How I will completely understand if you don't want these refactorings! Please just let me know which to get rid of/keep (or if you hate all my refactorings, please just say so!) and I will make a new cleaner PR.

Also, I didn't update the readme in this PR because it was more of a "take a look at this" commit ... but once you're ok with the final PR I'll update the readme to explain how to provide a custom component.

cometkim commented 3 years ago

This looks a bit hacky to me.

Gatsby provides component shadowing to support this kind of extension case. By just separating the Modal Container component path, users can easily override it.

machineghost commented 3 years ago

You are more than welcome to submit a PR for a less "hacky" solution that uses component shadowing (and I'd have nothing against it!) ... but if you're not going to then let's not let perfection be the enemy of good here. This PR breaks nothing and adds a desired feature.

P.S. And those refactorings were also meant to help facilitate other improvements that have been requested. But honestly at this point I'm more apt to fork the library if I'm going to make those improvements (both because the maintainer here is MIA, and because it's really a different library when you allow a choice of any modal library, because then React-Modal doesn't need to be a dependency) ...

... so, again, I'd have nothing against someone submitting a "less hacky" (in your estimation) PR here.

P.P.S. One last note: it's not just providing the path to the component though. That component also needs "modal props" (ie. custom information from build time in gatsby-node). Not knowing what component shadowing even is, I can't say whether it can solve that part also or not.