-
Notifications
You must be signed in to change notification settings - Fork 0
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
(components) feature: upgraded the Modal to @wethegit/react-modal v3 #350
Conversation
…le with renderTo prop
🦋 Changeset detectedLatest commit: 082a0e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @pravton! Couple notes from me, but we also need to clean up the docs:
This sentence is incorrect (there's no trigger
prop on the Modal):
The component takes a trigger prop that is a function which receives a toggle function as its only argument and should return the element that will trigger the modal.
Can we just re-word some of these docs to explain the usage better, i.e. how the useModal
hook returns the props required for the Modal itself (toggle
and triggerRef
), as well as the isOpen
state.
…addded renderTo to the prop list instead of using spread operator
@andrewrubin I have also updated the readme and the example and ready for you when you have a moment! |
It's looking good to me @pravton. I think we're ready for a changeset on this one 🎉 |
Description
Upgraded the Modal component to use the latest version (v3) of @wethegit/react-modal which addresses the issue #318
Task: https://wethecollective.teamwork.com/app/tasks/22769488