Skip to content
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

fix(Modal): set display block always, fixes #3399 #3571

Merged
merged 2 commits into from Apr 11, 2019

Conversation

@kt3k
Copy link
Contributor

kt3k commented Mar 26, 2019

We started setting display: block to div.modal at onEnter timing of Transition at Pull #3187. But when we set animation={false} to Modal, the Transition object is not created and therefore onEnter (=handleEnter) is not called. This causes empty modal when animation set false (I think this is the issue #3399 ). This PR tries to fix this behavior by setting the modal's display style to block always.

I think this doesn't break existing behavior because as @jquense says

we don't allow modals to be in the DOM prior to showing

the modal's dom element doesn't exist before rendering. So I think it's safe to set display always block.

@@ -340,7 +338,7 @@ class Modal extends React.Component {
onExiting,
manager,
ref: this.setModalRef,
style: { ...style, ...this.state.style },
style: { ...style, display: 'block', ...this.state.style },

This comment has been minimized.

Copy link
@taion

taion Mar 26, 2019

Member

i think it'd be best to do this only when animation is disabled

This comment has been minimized.

Copy link
@kt3k

kt3k Mar 27, 2019

Author Contributor

OK, I'll update.

@taion
taion approved these changes Mar 27, 2019
@taion taion merged commit 0cd1c30 into react-bootstrap:master Apr 11, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@kt3k kt3k deleted the kt3k:fix_3399 branch Apr 12, 2019
@prakis

This comment has been minimized.

Copy link

prakis commented Jul 9, 2019

I still see this issue, my module versions are
"react-bootstrap": "1.0.0-beta.5",
"bootstrap": "4.3.1",

Thank you

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Jul 10, 2019

@prakis This change was first included in v1.0.0-beta.7. You need to update to that version or above.

@prakis

This comment has been minimized.

Copy link

prakis commented Jul 10, 2019

@kt3k Thank you. I am testing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.