Modal #224
Modal #224
Conversation
|
@Matt-Butler In regards to the warning thats being logged, do we know what code / commit is causing this. I'm curious how the CI tests pass if this is being logged, unless its a console.warn statement that is triggered instead of a console.error statement. How is this error triggered? Edit:
Trigged by opening a modal and then clicking the browser back button or clicking a link to a new route. Seems the issue is related to modal code + react router updating routes.
Tested with React 15.5, the warning still occurs when following the steps above. We are also seeing a similar warning with react-datepicker. With webpack-dev-server running, navigate to the datepicker. Change the date and you will see a warning in the console. |
packages/terra-modal/src/Modal.jsx
Outdated
| **/ | ||
| isOpened: PropTypes.bool, | ||
| /** | ||
| * String that labels the modal for screen readers |
There was a problem hiding this comment.
I think this comment is out of date.
packages/terra-modal/lib/Modal.scss
Outdated
| @import '~terra-mixins'; | ||
|
|
||
| .terra-Modal { | ||
| background-color: #fff; |
There was a problem hiding this comment.
Should we enforce the use of terra-legacy-theme colors?
https://github.com/cerner/terra-core/blob/master/packages/terra-legacy-theme/src/_palette.scss#L67
packages/terra-modal/lib/Modal.scss
Outdated
| overflow: hidden; | ||
| position: fixed; | ||
| right: 10px; | ||
| top: 10px; |
There was a problem hiding this comment.
Was a decision made about using px units vs rem? Will we just swing back through and use the new mixin for converting these when it's available?
#164
There was a problem hiding this comment.
We should be using rem units. When the rem mixin is available we can start using pixel units in the mixin and output rems.
packages/terra-modal/package.json
Outdated
| "classnames": "^2.2.5", | ||
| "focus-trap-react": "^3.0.2", | ||
| "terra-mixins": "^1.0.0", | ||
| "react-portal": "^3.0.0" |
There was a problem hiding this comment.
Does this component need an explicit terra-base dependency?
#251
packages/terra-modal/src/Modal.jsx
Outdated
| /** | ||
| * Content inside the modal dialog | ||
| **/ | ||
| children: PropTypes.element.isRequired, |
There was a problem hiding this comment.
Similar to how we standardized the height / width properties I think children type is also something we should standardize. ( with maybe the exception of restrictive element children of a specific component type )
I believe most components specify the child type as node so that any renderable object can be passed into children. I think it becomes a little confusing when some components can render anything while other components will only accept element types as children.
|
|
||
| componentDidMount() { | ||
| // Disable background scrolling | ||
| document.body.style.overflow = 'hidden'; |
There was a problem hiding this comment.
Would the Overlay prevent any scroll events? Many Orion applications utilize a SlidePanel / fixed ContentPanel as a base component that manages its' own overflow, without the overlay I think these would still scroll. I think the current implementation has the full screen overlay prevent any scrolling.
There was a problem hiding this comment.
I'm not sure I quite follow. Are you proposing that when the modal is active, content in the background can still scroll?
Also, the current implementation enforces that there is always an overlay when a modal is active.
There was a problem hiding this comment.
I think content can still scroll even if you set this on body because body is not the only element that can have an overflow set, so I don't think this is an effective way to disable scrolling. But the Overlay can stop scrolling, does that make any sense?
|
|
||
| componentWillUnmount() { | ||
| // Enable background scrolling | ||
| document.body.style.overflow = null; |
There was a problem hiding this comment.
This could potentially cause some thrashing if the consuming application has styles set on the body
There was a problem hiding this comment.
This should only affect the overflow style property on the body. When do you think this would be an issue?
There was a problem hiding this comment.
I think this could cause unwanted side effects. As an example the base layout every Orion application uses currently has body overflow set to hidden. This will result in removing that style and potentially causing a lot of issues with the layout.
| import './ModalOverlay.scss'; | ||
|
|
||
| const propTypes = { | ||
| classNameOverlay: PropTypes.string, |
There was a problem hiding this comment.
Is there a good reason to keep this as an explicit prop rather than just spreading custom props onto the overlay?
I think you're using className above to pass the class, and not actually using this prop.
There was a problem hiding this comment.
I'm going to move the overlay class logic into overlay
| @@ -0,0 +1,3 @@ | |||
| # Themeable Variables | |||
|
|
|||
There was a problem hiding this comment.
If there are no themeable variables, this file can be removed.
packages/terra-modal/src/Modal.scss
Outdated
| @import '~terra-mixins'; | ||
|
|
||
| .terra-Modal { | ||
| background-color: $terra-white; |
There was a problem hiding this comment.
Use rem values for these pixel values.
With the 14px root font, 10px = 0.714rem. 5px = 0.357rem.
There was a problem hiding this comment.
I think we want to keep left, right, top, and bottom as 10px instead of rem. If the user updates the font size in their browser, we probably want the area around the modal dialog to stay consistent.
Large Font
Small Font
There was a problem hiding this comment.
Good point, yeah lets leave these position properties as PX units.
packages/terra-modal/src/Modal.scss
Outdated
| border-radius: 5px; | ||
| bottom: 10px; | ||
| display: block; | ||
| left: 10px; |
There was a problem hiding this comment.
Update this to use the position-start mixin: https://github.com/cerner/terra-core/tree/master/packages/terra-mixins/docs#terra-position-start--terra-position-end
There was a problem hiding this comment.
updated to use mixin
packages/terra-modal/src/Modal.scss
Outdated
| outline: none; | ||
| overflow: hidden; | ||
| position: fixed; | ||
| right: 10px; |
There was a problem hiding this comment.
Update this to use to position-end mixin: Update this to use the position-start mixin: https://github.com/cerner/terra-core/tree/master/packages/terra-mixins/docs#terra-position-start--terra-position-end
There was a problem hiding this comment.
updated to use mixin
| @media screen and (min-width: 48em) { | ||
| &:not(.terra-Modal--fullscreen) { | ||
| bottom: auto; | ||
| box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3); |
There was a problem hiding this comment.
These can stay as pixel values.
Summary
Code Review for Modal component
I wanted to get this out so people can start looking at it. I am still going to add documentation to this code review.
I am also seeing this warning in the console when viewing nightwatch tests. I will fix that as well.
@cerner/terra