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

Attempt to enable custom modal components #54

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

Conversation

machineghost
Copy link

Fix for: #52

@machineghost
Copy link
Author

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
Copy link

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
Copy link
Author

machineghost commented Nov 11, 2020

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.

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.

None yet

2 participants