-
Notifications
You must be signed in to change notification settings - Fork 3
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
added immersive and regular modals #39
Conversation
docs/immersiveModal.md
Outdated
<Button onClick={this.onOpenDefaultModal}>Open ImmersiveModal</Button> | ||
{defaultIsOpen && ( | ||
<ImmersiveModal onClose={this.onClose}> | ||
<ImmersiveModal.Title>This is styled with ImmersiveModalTitle</ImmersiveModal.Title> |
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.
probably want to update the text to say "This is styled with ImmersiveModal.Title
". Similar comment for the rest of the file
docs/immersiveModal.md
Outdated
### Proptypes | ||
| prop | propType | required | default | description | | ||
|----------|--------------------|----------|---------|------------------------------------------------------------------------------------------------------------------------------| | ||
| onClose | func | no | () => {} | function is that executed when either the close icon is clicked or any part of the page outside the modal is clicked | |
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.
function that is*
docs/modal.md
Outdated
### Proptypes | ||
| prop | propType | required | default | description | | ||
|----------|--------------------|----------|---------|------------------------------------------------------------------------------------------------------------------------------| | ||
| onClose | func | no | () => {} | Function is that executed when the close icon is clicked | |
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.
function that is*
src/constants/keycodes/index.js
Outdated
enter: 13, | ||
escape: 27, | ||
space: 32, | ||
h: 104, // used for hugpacks |
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.
We can strip out the ones we don't need. Probably only need escape
|
||
this.htmlNode = document.querySelector('html'); | ||
this.domNode = | ||
document.querySelector('#reactPortalSection') || document.body; |
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.
just curious what happens if query selector is null. It seems from the code that it's not a hard fast requirement to have a node for the react portal
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.
Portals: https://reactjs.org/docs/portals.html
The domNode here is the container for the portal. If we don't pass it a node, it definitely breaks (blank screen, console error). So in this case, if the query selector is null, we need to have a default node (doc.body) to set as the container for the portal so the page doesn't break entirely.
} | ||
|
||
componentDidMount() { | ||
this.htmlNode.classList.add('no-scroll'); |
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.
just wanted to check if no-scroll
class was part of the inject global styles
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.
Confirmed -- just checked in radiance, gatsby, and main app and it's the same in all three.
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.
🙌
@@ -0,0 +1,126 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import ReactModal from 'react-modal'; |
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.
as a follow up (not high priority), i'd like for us to refactor this modal to not use ReactModal. I don't think we need it and we're doing the immersive modal without it.
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.
Created an issue for follow up: #40 (comment)
@snags88 Updated! |
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.
👍
Trello: https://trello.com/c/5arVBzoV