breaking(Modal): Fix vertical position with SUI 2.3 #2690
Conversation
|
Ran out of time tonight to update the tests. Will try to get some eyes on them early tomorrow... but I likely will not have time to get back in on them until the evening. |
Codecov Report
@@ Coverage Diff @@
## next #2690 +/- ##
==========================================
+ Coverage 99.67% 99.67% +<.01%
==========================================
Files 161 161
Lines 2762 2769 +7
==========================================
+ Hits 2753 2760 +7
Misses 9 9
Continue to review full report at Codecov.
|
src/addons/Portal/Portal.js
Outdated
|
|
||
| this.mountPortal() | ||
|
|
||
| // Server side rendering | ||
| if (!isBrowser()) return null | ||
|
|
||
| this.rootNode.className = className || '' | ||
| this.rootNode.style = style ? toInlineStyle(style) : '' |
There was a problem hiding this comment.
We also no longer need this external library based on the refactor requested below.
src/modules/Modal/Modal.js
Outdated
| @@ -364,6 +372,7 @@ class Modal extends Component { | |||
| onMount={this.handlePortalMount} | |||
| onOpen={this.handleOpen} | |||
| onUnmount={this.handlePortalUnmount} | |||
| style={{ display: 'flex !important' }} | |||
There was a problem hiding this comment.
I see a problem there, React doesn't support important keyword for inline styles, see facebook/react#1881. Take a look on Dimmer's PR #2656 for a workaround.
There was a problem hiding this comment.
Ah. I guess it is working in this case because this is being applied to a rootNode of a portal. Or possibly because the toInlineStyle package is handling it. I'll take a look today.
There was a problem hiding this comment.
@layershifter Updated. Technically it was working because we were passing those attributes as a string. This will keep the semantics the same way as we are handling this use case in Dimmer.
|
|
||
| this.mountPortal() | ||
|
|
||
| // Server side rendering | ||
| if (!isBrowser()) return null | ||
|
|
||
| this.rootNode.className = className || '' | ||
| this.rootNode.style = style || '' |
There was a problem hiding this comment.
I think this is susceptible to users passing style objects to the <Portal style={...} />. Let me know if I'm misreading here.
There was a problem hiding this comment.
@levithomason correct. It is. Is there a reason you see that we should not allow that? If we do not want to allow that, is there another way you would prefer we do this?
There was a problem hiding this comment.
Hm, I think I was tripping out that day... Apologies, continue!
|
@layershifter @brianespinosa this looks good to me. Please merge at will if you are good with it 👍 |
* chore(package): update SUI to 2.3 * feat(Dimmer): update SUI to 2.3, add verticalAlign (#2656) * feat(Transition): update SUI to 2.3, add `zoom` and `glow` (#2658) * feat(Icon): update SUI to 2.3 (#2665) * breaking(Modal): Fix vertical position with SUI 2.3 (#2690) * Update SUI css to 2.3.1 * Disable negative top margin calc * Add the ability to apply inline styles to Portal * Top Aligned modal example * Make sure we are not overriding the top margin inline * type defs updated for Portal * Type update for Modal * Refactor to use setProperty instead of string for important * Remove inline-style lib to handle string differently * chore(gulp): remove task with icons * feat(Icon): regenerate icons * docs(Usage): add warning about SUIR versions * docs(README): remove note * fix lint error * fix(iconSearch): dedupe merged name sets
This is part of the work for #2550. There are a few things here to look at.
centeredprop that can be set to false which positions the modal to the top of the viewport instead of the center. The default centered modal follows the same logic as SUI.There may be more to do but I at least wanted to get this up so we could get some eyes on it. I think we may be able to ship out an update that moves us for supporting 2.3.x soon after this one.