Modal improvements #810
Modal improvements #810
Conversation
|
the header of the third commit: |
jquense
force-pushed
the
modal-improvements
branch
from
82b3374
to
89421fd
Compare
|
This will resolve #354, BTW. |
| @@ -1,5 +1,13 @@ | |||
| import React from 'react'; | |||
|
|
|||
|
|
|||
| let canUseDom = !!( | |||
|
And #723, I believe. Very nice. |
| function onFocus(context, handler) { | ||
| let doc = domUtils.ownerDocument(context); | ||
| let useFocusin = !doc.addEventListener | ||
| , remove; |
There was a problem hiding this comment.
In generall we haven't been following a single var pattern in this library. For consistency can this just be another let definition. Webpack Uglify plugin should reduce it to single var in the optimized bundle.
|
This all looks really good, minor style tweaks from single var pattern for consistency and it should be ready to go. |
|
I will make that change :) there is one thing that bothers me here that I don't know how to fix given the current way overlays work.
Generally I have solved things like this by creating something like the TransitionGroup addon that will keep a component child mounted (by caching it in state) until the transition completes, and then unmounts, but I am not sure that would work here, since the Overlay's don't ask the component if they should be rendered or not. |
|
Latest CI failures should resolve if you rebase on the latest master |
Correctly pads the modal to account for the container having a scroll bar
Allows you to configure whether the modal should enforce focus when open. Ideally this doesn't need to be a public API, but in the case of static models (like in the docs) you need to turn it off, because you can't assume its the only one open on the page.
Incorporates upstream tbs logic to pad the container before hiding the scrollbar with the `.modal-open` class fixes #354
jquense
force-pushed
the
modal-improvements
branch
from
db6fb46
to
a0034e1
Compare
|
@mtscout6 I don't think it blocks this PR, just noting a known limitation to the fix here that doesn't bring it completely inline with upstream bootstrap. I 'd be happy to fix it before we merge if anyone can help figure out a simple way to address it, otherwise we can revisit when #810 is done. One thought was just to have FadeMixin call an optional callback when it has faded out, but I wasn't quite sure how to safely implement that without breaking compat |
|
I'd say this is a smaller scoped problem that can be solved in another issue. |
| }; | ||
| }, | ||
|
|
||
| getInitialState(){ | ||
| return { }; |
There was a problem hiding this comment.
empty ?
itsn't it the same as there is no getInitialState() at all ?
There was a problem hiding this comment.
It is pretty much the same yeah, with one useful difference: providing an empty object lets you access properties on it for a falsey value instead of an error (Null Exception)
// without the empty object in getInitialState this would throw the first render
var style = this.state.dialogStyles // state is nullI could be slightly more correct and set state to { dialogStyles: undefined } but it seems unneeded
There was a problem hiding this comment.
Ah.. I see.. I should try it myself. Remove it and see what's happening. 😊
|
LGTM Is there anything else preventing this from getting merged today? |
|
not that I know of :) |
|
|
First few bits of getting the Modal logic up to par with upstream tbs. No breaking changes, no major api rewrites...yet