feat(Modal): Support fade and timeout props in the Modal component to allow configuring + disabling of the fade effect #339
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
annejohnson
force-pushed
the
add-modal-fade-prop
branch
from
631e922
to
4b85158
Compare
- if fade=false, the following happens: - no 'fade' class is applied - the modal + backdrop transition timeouts become 0 - the transitionAppear, etc. booleans passed to Fade are false
annejohnson
force-pushed
the
add-modal-fade-prop
branch
2 times, most recently
from
1785b5e
to
e75bb5c
Compare
- This feels like a better design. If we are not doing
a fade effect, why use a Fade component?
- This also gets rid of the usage of TransitionGroup
when fade={false}.
- Ensure onEnter and onExit fire when expected.
annejohnson
force-pushed
the
add-modal-fade-prop
branch
from
e75bb5c
to
4acf9b8
Compare
- Extract modalDialog rendering into a separate method - Simplify the variable setting in #renderChildren following the movement of several of them into #modalDialog
annejohnson
force-pushed
the
add-modal-fade-prop
branch
from
1595c79
to
6b9af07
Compare
annejohnson
changed the title
annejohnson
changed the title
src/Modal.js
Outdated
|
|
||
| if (this.hasTransition()) { | ||
| const modalTransitionTimeout = 300; | ||
| const backdropTransitionTimeout = 150; |
There was a problem hiding this comment.
Should these times be configurable, or should that be another PR. At a certain point we could make the transition timing variable and when it is 0 disabled the transition animation.
There was a problem hiding this comment.
I thought that was a good idea. I added that in this commit: a2fb915
Let me know what you think. Thank you for taking a look!
added 2 commits
March 4, 2017 18:28
…e as props - Update docs - Add test
annejohnson
changed the title
|
Could this be merged? I really need this. |
|
@annejohnson I unfortunately messed up by not merging this first. A different pr merge caused a conflict with this branch. Can you resolve the conflict and commit that change? Thanks and sorry about that! |
Conflicts: src/Modal.js
|
@eddywashere I resolved the conflict. Sorry for the delay. Thanks for taking a look at these updates! |
|
Can we merge this please? Seems like its solid. |
TheSharpieOne
approved these changes
May 5, 2017
Dohxis
added a commit
to Dohxis/DefinitelyTyped
that referenced
this pull request
Aug 24, 2017
PR [DefinitelyTyped#339](reactstrap/reactstrap#339) adds a new prop for Modal component.
Merged
8 tasks
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 adds support to the
Modalcomponent for afadeprop of type boolean (default value: true). It also adds support for modal+backdrop appear/enter/leave timeout props.It addresses the following issues: #168 & #352
If a value of false is passed for
fade, the Bootstrap modal appears instantaneously, without a fade transition. Likewise when the modal closes.The timeout props control the speed of the fade effect for the modal and the backdrop.
Also included in this PR are test cases and an example in the documentation.