fixed modal close issue #5865
Merged
fixed modal close issue #5865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
williaster
reviewed
Sep 12, 2018
| @@ -82,31 +82,31 @@ export default class ModalTrigger extends React.Component { | |||
| 'btn btn-default btn-sm': this.props.isButton, | |||
| }); | |||
| if (this.props.isButton) { | |||
| return ( | |||
| return [ | |||
There was a problem hiding this comment.
do you think we should use React.Fragments here? I'm not sure of the advantages of them vs arrays (maybe <>...</> syntax is supported with our build config?)
Did you check the usage as a MenuItem? I was wondering if the parent of a MenuItem (prob a <ul />) would care if it had a modal / non-MenuItem (prob an <li />) child . This might not be a problem if the modal renders into a portal (not the parent dom container) since it wouldn't create an invalid DOM structure.
EDIT: you prob did check the latter point if you were testing the "view query" modal 👍
There was a problem hiding this comment.
framents vs arrays described here. Not sure it matters too much.
There was a problem hiding this comment.
@williaster Thanks! was reading the article before. and Indeed In think in this case, <Fragment /> makes more sense.
Children in an array must have a key to prevent React’s key warning.
There was a problem hiding this comment.
@williaster and yes, I checked the MenuItem :)
|
Related to React16 upgrade? |
|
yeah I think so |
williaster
approved these changes
Sep 12, 2018
conglei
force-pushed
the
conglei_modal_fix
branch
from
77ec029 to
2dd4c69
Compare
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Sep 21, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Sep 21, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Sep 21, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Sep 21, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Sep 21, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
|
@conglei does this PR solves the issue where you can't write in textfield or use select2 and some checkboxes inside modal
|
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 11, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 12, 2018
* fixed modal close issue * change Children array to Fragment
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 12, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 12, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 17, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 17, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 17, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 17, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 29, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 30, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 30, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 30, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 30, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
betodealmeida
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Oct 30, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Nov 2, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
youngyjd
pushed a commit
to lyft/incubator-superset
that referenced
this pull request
Nov 2, 2018
* fixed modal close issue * change Children array to Fragment (cherry picked from commit 10836ce)
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is to fix the issue that the modal generated by
ModelTriggercannot be closed properly.Credit to @williaster for finding the bug!
The root cause is
this.openwill be trigger since the modal literally is inside the parent dom which hasthis.openbinded withonClick.