-
Notifications
You must be signed in to change notification settings - Fork 30
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
No Way to Style Modal or Use a Different Modal #52
Comments
👋 hi @machineghost! This sounds like a great idea. I originally had a similar thought where I wanted to make react-modal an optional dependency. If you want to put together a proposal or PR for how this would work I’d happily take a look. |
PR submitted. As noted, I refactored the library to understand it better, but I understand that refactorings are highly subjective and you may not want them. I submitted them initially to get feedback, but PLEASE feel free to provide feedback of "these refactorings are horrible, please get rid of them entirely and just submit the custom modal stuff". |
I should note that the only reason I'd even want to keep the refactorings (well, aside from the fact that I think they might make the code clearer to a future editor) is that I'd also like to make another improvement to this library: I'd like to make it possible for you to define a "background page", so that when someone navigates directly to the modal's URL, it can still show the original page in the background. Keeping the refactorings would just make the code more familiar and help facilitate doing that, but again I am NOT wedded to them in any way, and if you literally reply to this with just a single sentence ("not a fan") I'll happily redo my commit without them (and no feelings hurt, I promise!) But on the very off-chance you see them and think "that made sense", I just wanted to get your two cents first. |
Upping this topic. I'm using my own custom |
You can just my repo if you want; it has this functionality already. Of course, it has the downside of not getting any new updates ... but then of course this repo doesn't seem to be actively maintained either, so maybe that doesn't matter? In any case, to use my repo just change the version in package.json to the repo URL:
(and then of course run |
@machineghost thanks a lot. This looks pretty cool! Do you know how i can get rid of |
First off, sorry: I realized I had a typo (an extra "g" in my name) in my URL. I edited and fixed it to:
but I imagine you noticed and fixed it yourself if you got things working (if not, please correct it and re- Second (for you or anyone else reading this), I should have clarified how to use it. In
Then in
As for:
The idea is ... any way you want :) You could, for instance, use the Styled Components library to "bake the CSS into" your own component, or you could use the React CSS Modules library to do it with dynamic classes, or you could use inline
You can see the full scope of what I actually changed in a single commit: As you can see at the bottom of that link, the render method really isn't creating extra |
@machineghost this is perfect! The only thing i haven't figured out yet is how to pass additional props except for the ones you mentioned. Great job mate!! |
Sorry, I left out the
In the above example your component would get passed a But at "run-time" you just have to use mechanisms you control (hooks, context, etc.) to pass data in. This is because any data (eg. the title) has to somehow live beyond memory/state. In other words, you could just use, say, link state, on the link to open the modal. This is how the library passes the In general you'll want to derive your modal's title from your URL, and you can do that without passing any title in: just check P.S. Also the dialog templates can use Gatsby queries and take a Inside your modal component you could use the ID from the URL to find the car from your graph data with the matching ID, and then display a title of "Buy that car name Dialog". |
hey @machineghost, thanks a lot for the comprehensive explanation! I made everything work, all looks good. Here is my gatsby-config.js:
I access the objects inside
All works perfect on development but throws this error on I tried accessing first P.S. |
First off, I take it that this line:
was just an approximation. From the rest of the code, I imagine it's actually more like this?
If so, it sounds like whatever value you're providing (
No, but you can include static values in the URL, and then both the inner-page modal and the page modal will have access to them. For instance, if
Of course, there are limits to how much info you can pass this way. Also, you have to URL encode/decode it (eg. with If you need anything more complex, you should keep the more complex data in your GraphQL query, and just put the IDs of that data in your URLs. |
@machineghost yeah you're right, that was a problem with the router. I encountered a bug, which i think has to do with this module. When i test on build / compiled version, on the first run, when opening a link that has |
I'm unable to reproduce that: on my end everything works fine, even on the first click, in a built version of my site. Here's the code involved (the
As you can see, it's just using the props it gets (specifically the |
I've already covered this in #60, but here's my take on this, just use a hook: const [modal, closeTo] = useModalRouting({
modalProps: {
contentLabel: "Custom content label",
onAfterOpen: () => {
console.log("onAfterOpen")
},
closeTimeoutMS: 300,
style: { overlay: {}, content: {} },
id: "some-id",
onRequestClose: () => {}, // would error, is an internal-exclusive prop
},
})
//..
return (
<div>
{modal ? (
<Link to={closeTo}>Close</Link>
) : (
<header className="mb-2">
<h1>Website Title</h1>
</header>
)}
<h2>Modal Page</h2>
<Link to="/">Go back to the homepage</Link>
</div>
) This allows For now, I don't see any limitations on doing it like this. I also still haven't tried adding support for a custom modal component since I currently don't plan to use another one ( All that's left, then, is to implement these: #47-#29, #58, as I think it there are cases where these are particularly useful. Anyway, if anyone's interested here's a first look at the hook: decanTyme/gatsby-plugin-modal-routing@master...decanTyme:feat/expose-react-modal-props-hook |
@decanTyme I noticed you haven't published your solution yet, any plans when are you going to push so I can npm i the latest version? |
Already published here: https://www.npmjs.com/package/@decantyme/gatsby-plugin-modal-routing Haven't really had time to do stuff on it tho so it might still have some bugs to be ironed out. |
Currently
gatsby-plugin-modal-routing
assumes that you want to use a stockreact-modal
modal. However, it could be useful if the plugin provided a way to use other modal components, such as a Styled Components-styled version of thereact-modal
, or possibly even a custom component that uses another modal library (eg. some UI frameworks provide their own modals).I briefly looked at the code, and it seems the only place
react-modal
is actually used is inReplaceComponentRenderer
. Why not have a plug-in option calledmodalComponent
(or justmodal
?) which replacesreact-modal
, but leavereact-modal
as the default value of that option?Would you accept a PR implementing this?
The text was updated successfully, but these errors were encountered: