Update react transition group (WIP) by amikhalev · Pull Request #557 · reactstrap/reactstrap

amikhalev · 2017-08-29T06:20:06Z

No description provided.

virgofx

Looks good and simplied much of the code. I think if we just had more clear cut documentation on the default timeouts and if changing them, what variables need to be set that would be my only suggestion.

virgofx · 2017-08-29T13:29:19Z

-  transitionEnter: true,
-  transitionLeave: true
+  baseClassActive: 'show',
+  timeout: 150,


It appears bootstrap default is 300ms

https://github.com/twbs/bootstrap/blob/v4-dev/scss/_variables.scss#L688

We should definitely comment this (briefly here and more thoroughly in the example/documentation) as the timeout here also requires updating $modal-transition in custom bootstrap builds as this is important for syncing the CSS UI with the timeouts here.

EDIT: I apologize. I referenced the Modal timeouts against the Fade here. Bootstrap's Fade components are all 150 so this is correct. The modal is separate which uses 300ms/150ms. Could we at least add documentation in both these locations in source and in documentation examples as its real important that these sync otherwise there will be huge jumps where the transition end timeouts prior or the CSS transition is still continuing.

Modal timeout changes need to update: $modal-transition

Fade component changes (e.g. Alert) need to update: $transition-fade

amikhalev · 2017-08-30T14:17:41Z

@TheSharpieOne

I think this is almost done. Only Carousel doesn't work ATM. I do have a few questions

I kinda forgot to do conventional commits, so would you like me to go back and edit each commit message? Or I could squash this into 1 or several commits with better messages.
Should I add a doc page for the Fade component? Or is this considered an internal component
Any other changes that need to be made to this code?

TheSharpieOne · 2017-08-30T16:51:41Z

@amikhalev Thanks a ton for tackling this.

I can (and will) squash when I merge the commit and change the commit message to use a conventional message
I would say yes, create a doc page, but if you don't, it probably isn't that big of a deal (like tether is)
- The transition prop names exposed for Alert vs Modal and CarouselItem. It would be nice to have them all be consistent.
- Make all of the transition props overridable (there are a few which are not)
- Allow timeout to an object or an integer much like Transition allows (kinda goes with consitent prop names b/c Alert has two timeout props)
- Speaking of the Fade component, would it be possible to use Fade with Alert rather than having it's own within the component?
- When checking the status of the transition, can you use the variables exposed by Transition ?import Transition, { EXITED, ENTERED, ENTERING, EXITING } from 'react-transition-group/Transition';
- What is going on with Carousel? (do you plan on resolving it in thie PR or should this be merged and Carousel be addressed in a later effort?)
- I'll add more comment inline

TheSharpieOne · 2017-08-30T16:54:43Z

-        {children}
-      </div>
+      <Transition
+        in={true}


Do we want expose Transition props and spread them here? If so, can you also check for the existance of a user provided onExited and onExiting (via props) and call them in your callbacks (to not clobber the user's callbacks)

virgofx · 2017-08-31T23:07:04Z

Best practice is to not cherry pick. We don't do that anywhere. Why was that changed?

src/Carousel.js
https://github.com/reactstrap/reactstrap/pull/557/files#diff-4015e3ba4b5aea6acd9696987286fcfdR4

src/CarouselItem.js
https://github.com/reactstrap/reactstrap/pull/557/files#diff-e12c2aefc69d2a3602a9370a2d8aa857R4

src/Fade.js
https://github.com/reactstrap/reactstrap/pull/557/files#diff-8f766132cbd9f8de55ee05d63d75abd8R4

Use proper ES imports.

mkalish · 2017-08-31T23:30:11Z

+      >
+        {(status) => {
+          const { direction } = this.context;
+          console.log("altText: ", altText, ", status: ", status, ", direction: ", direction);


Extra log statement here

mkalish · 2017-08-31T23:34:53Z

@amikhalev
I'm happy to take a look at updating the carousel code over the weekend if you want to get this merged

amikhalev · 2017-08-31T23:42:02Z

@virgofx Ok, I wasn't aware that individual imports was preferred

amikhalev · 2017-08-31T23:43:39Z

@mkalish @TheSharpieOne I should be able to fix Carousel (and the rest of your issues), I just made some temp commits for now. I'll probably get it done some time this weekend

virgofx · 2017-08-31T23:48:12Z

@amikhalev Thanks. :)

TheSharpieOne · 2017-09-01T21:55:13Z

@amikhalev I don't want to mess with your flow, but I have some large commits ready to be merged into v5 (such as the popper branch). I had to update react-scripts which updated eslint and jest which changed the test API (the jest 19/20 stuff) and eslint rules. Everything is working in the popper branch and test coverage have been improved (back to 99.66%), but I had to touch lots of test files, including ones you have touched. I also had to fix new linter issues which touched some of the code you have touched.
I'm not sure the best course of action, if you want me to merge my stuff in and you rebase / pull and deal with what conflicts may happen (since you will be more familiar with those files as the major changes are in your branch), update tests which may need to use the new jest stuff, as well as fix linter issues which may arise. Or I can wait to merge my code until you are done and I can deal with the conflicts (most likely keeping your changing and just fixing anything that breaks).

amikhalev · 2017-09-01T22:02:27Z

@TheSharpieOne Go ahead and merge the updates, I can deal with any merge conflicts

amikhalev · 2017-09-02T06:06:33Z

@virgofx It doesn't look like there's a way to access the constants defined here without importing "react-transition-group/Transition". Would you say it is OK to do so in this case?

amikhalev · 2017-09-02T08:32:20Z

@TheSharpieOne

I think I fixed most of your issues. Carousel works now (:tada:), Alert, Carousel and Modal all expose their transition props, Alert uses Fade, and everything uses constants from react-transition-group

Here are the issues that I think I still need to address

There is some kind of issue with Modal which prevents it from dismissing properly
If you click the control button or keyboard button too quickly with Carousel, the transition breaks and it shows multiple slides stacked vertically
Lots of documentation is out of sync (mostly for Carousel)
Lots of tests are broken (might wait to fix this one until new testing stuff is merged)
Probably should write more/better tests for Fade, Carousel (esp. slide prop)
I think that since Fade is used by both Alert and Modal, plus it could be used by users of reactstrap for fading arbitrary components, it should probably get it's own documentation page explaining it's props in more detail

amikhalev · 2017-09-03T22:06:14Z

@TheSharpieOne It looks like Collapse could be made to use Transition. Should I pass it's existing props to Transition or just update them to match?

virgofx · 2017-09-04T16:43:15Z

I was thinking the same thing. I believe we could just wrap around Transition and the Collapse component itself should be much smaller code-wise as we should re-use much of the transition logic/props, etc. So the closer we can keep inline with transition would be my vote.

amikhalev · 2017-09-04T17:41:40Z

In that case, what should I name props?
in or isOpen?
onOpened or onEntered?
onClosed or onExited?
Should they be consistent between Alert, Fade, CarouselItem, Collapse and Modal? Or maybe more than one should be supported for each component. I'm not sure what's best here

TheSharpieOne · 2017-09-09T15:02:12Z

I think we should probably keep isOpen, even though in is more generic. In the few cases where isOpen doesn't make any sense, I'm fine with using in there (even if it isn't 100% consistent). For the callbacks, I would prefer the same names as the transition component.

Removed global 'react-transition-group' from both rollup and webpack configs

Also updated props related to transitions in order to better match updated react-transition-group props

This meant adding FadePage, plus adding to routes and navigation. I also updated prism to add the autolinker plugin to enable links in the docs for proptypes

…ion can be used instead

holgersindbaek · 2017-09-15T11:41:23Z

When is this branch getting merged? I'm getting a lot of issues with react-transition-group.

carlschubert · 2017-09-15T16:26:09Z

I've also been watching this thread. I've been holding off on a few things waiting for react-transition-group to be updated. It would be nice to know the timeline.

amikhalev · 2017-09-15T16:37:53Z

All of the code is done. The only thing that it needs is updated tests (a lot of which is just removing tests because the code was simplified a lot). I'll probably get it done later today

amikhalev · 2017-09-16T00:05:09Z

Ok, looks like all tests pass now. I'm pretty sure all docs are updated too. I think this is ready for final review

TheSharpieOne

Docs could use some work, but I don't know how much the existing docs fit into the WIP docs eddywashere is working on. Other than that there are just a few things which can be addressed after it's merge (I'll make the tweaks).

TheSharpieOne · 2017-09-16T02:08:57Z

-    return delay;
+  onExiting(node) {
+    // getting this variable triggers a reflow
+    const _unused = node.offsetHeight; // eslint-disable-line no-unused-vars


I wonder if the minifier will remove this or not

TheSharpieOne · 2017-09-16T02:10:31Z

-      }, this.getDelay('hide'));
-    }
-    // else: do nothing.
+    ['onEntering', 'onEntered', 'onExit', 'onExiting', 'onExited'].forEach((name) => {


Interesting. Maybe CarouselItem could do the same.

TheSharpieOne · 2017-09-16T03:07:48Z

Awesome job @amikhalev. Thanks a ton!

amikhalev · 2017-09-16T05:33:50Z

Thanks! It was my pleasure. reactstrap is a great library, glad to contribute!

holgersindbaek · 2017-09-16T13:00:47Z

Nice. Thanks.

* Updated rollup and webpack configs Removed global 'react-transition-group' from both rollup and webpack configs * Updated Alert for [email protected] Also updated props related to transitions in order to better match updated react-transition-group props * Updated AlertPage.js docs for new transition props * Fixed Alert tests for new transition props * Fixed lint issues related to Alert changes * Almost fixed Fade and Modal to work with [email protected] * Fixed Modal exit transition * Fixed lint issues * Fixed Fade test * Fixed Modal test; removed use of setImmediate * Fixed Modal.spec.js formatting * Updated install docs to remove react-transition-group * Updated modal documentation * Almost got carousel to work * Fixed fade props passing * Changed the way Carousel works; still not quite fixed * More attempts at fixing Carousel * Changed Alert to use Fade * Fixed all react-transition-group imports to not cherry pick * Now modal is broken :/ Fixing this * Some mork work * Changed Transition uses to use constants instead of strings * Made Carousel work! Plus added Carousel slide prop * Minor change to Fade test * Removed debug logging from CarouselItem * Fixed Modal and siplified logic * Extracted transition timeouts to utils * Simplified CarouselItem transition logic * Removed unnecessary noop props (they exist in Transition.defaultProps) * Removed CarouselItem status field * Added docs for Fade This meant adding FadePage, plus adding to routes and navigation. I also updated prism to add the autolinker plugin to enable links in the docs for proptypes * Updated Alert and Modal docs to point to Fade docs * Added mountOnEnter for Modal backdrop * Updated carousel proptypes doc * Fixed eslint issues * Updated Collapse to use Transition * Removed confusing callbacks from Modal. The callbacks on modalTransition can be used instead * Updated react-transition-group in package.json * Fixed collapse tests * Fixed eslint issues * Fixed Collapse callback issue, updated test, updated examples * Updated Collapse Docs * Added some Alert tests * Updated Collapse and Collapse tests * Updated Carousel tests * Updated Fade tests * Fixed modal tests

TheSharpieOne requested a review from eddywashere August 29, 2017 12:52

virgofx reviewed Aug 29, 2017

View reviewed changes

TheSharpieOne mentioned this pull request Aug 30, 2017

react-transition-group 2.x causes an unknown props error on Modal open #502

Closed

TheSharpieOne reviewed Aug 30, 2017

View reviewed changes

mkalish reviewed Aug 31, 2017

View reviewed changes

amikhalev force-pushed the update-react-transition-group branch from 1a67cbf to 12937d2 Compare September 3, 2017 18:09

TheSharpieOne mentioned this pull request Sep 9, 2017

feat(popperjs): use popperjs instead of tether #561

Merged

amikhalev added 8 commits September 11, 2017 16:38

Updated rollup and webpack configs

4103701

Removed global 'react-transition-group' from both rollup and webpack configs

Updated Alert for [email protected]

149841f

Also updated props related to transitions in order to better match updated react-transition-group props

Updated AlertPage.js docs for new transition props

7b8de79

Fixed Alert tests for new transition props

704ae36

Fixed lint issues related to Alert changes

a15a49f

Almost fixed Fade and Modal to work with [email protected]

ec4bbff

Fixed Modal exit transition

51f914a

Fixed lint issues

f876aa3

amikhalev added 7 commits September 11, 2017 16:38

Removed CarouselItem status field

b336935

Added docs for Fade

8c408a3

This meant adding FadePage, plus adding to routes and navigation. I also updated prism to add the autolinker plugin to enable links in the docs for proptypes

Updated Alert and Modal docs to point to Fade docs

0cda220

Added mountOnEnter for Modal backdrop

4aef835

Updated carousel proptypes doc

be227ff

Fixed eslint issues

b282c98

Updated Collapse to use Transition

21413f5

amikhalev force-pushed the update-react-transition-group branch from 76948b2 to 21413f5 Compare September 11, 2017 22:41

amikhalev added 5 commits September 11, 2017 16:52

Removed confusing callbacks from Modal. The callbacks on modalTransit…

84c8f4d

…ion can be used instead

Updated react-transition-group in package.json

654e17f

Fixed collapse tests

927c248

Fixed eslint issues

57f6133

Fixed Collapse callback issue, updated test, updated examples

f9e32f0

amikhalev added 6 commits September 15, 2017 16:03

Updated Collapse Docs

fcb7c9f

Added some Alert tests

ac07d82

Updated Collapse and Collapse tests

cdd75b5

Updated Carousel tests

71c63a4

Updated Fade tests

6fdbad1

Fixed modal tests

d6575b6

TheSharpieOne approved these changes Sep 16, 2017

View reviewed changes

TheSharpieOne merged commit e8737a5 into reactstrap:v5 Sep 16, 2017

zerkms mentioned this pull request Jul 19, 2018

@types/reactstrap Alert outdated transition properties DefinitelyTyped/DefinitelyTyped#27425

Closed

Uh oh!

Conversation

amikhalev commented Aug 29, 2017

Uh oh!

virgofx left a comment

Choose a reason for hiding this comment

Uh oh!

virgofx Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

amikhalev commented Aug 30, 2017

Uh oh!

TheSharpieOne commented Aug 30, 2017

Uh oh!

TheSharpieOne Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

virgofx commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkalish Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

mkalish commented Aug 31, 2017

Uh oh!

amikhalev commented Aug 31, 2017

Uh oh!

amikhalev commented Aug 31, 2017

Uh oh!

virgofx commented Aug 31, 2017

Uh oh!

TheSharpieOne commented Sep 1, 2017

Uh oh!

amikhalev commented Sep 1, 2017

Uh oh!

amikhalev commented Sep 2, 2017

Uh oh!

amikhalev commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amikhalev commented Sep 3, 2017

Uh oh!

virgofx commented Sep 4, 2017

Uh oh!

amikhalev commented Sep 4, 2017

Uh oh!

TheSharpieOne commented Sep 9, 2017

Uh oh!

holgersindbaek commented Sep 15, 2017

Uh oh!

carlschubert commented Sep 15, 2017

Uh oh!

amikhalev commented Sep 15, 2017

Uh oh!

amikhalev commented Sep 16, 2017

Uh oh!

TheSharpieOne left a comment

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne Sep 16, 2017

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne Sep 16, 2017

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne commented Sep 16, 2017

Uh oh!

amikhalev commented Sep 16, 2017

Uh oh!

holgersindbaek commented Sep 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

virgofx commented Aug 31, 2017 •

edited

Loading

amikhalev commented Sep 2, 2017 •

edited

Loading