Added ReactCSSTransitionGroup

[jasonzoladz]

No description provided.

[ethul]

Thanks for putting together this PR.

Overall, I think the code looks good. I do have a few comments, but I can touch on those later. Primarily, I am torn on if this addition would be better as an independent library. Originally, I didn't realize it would impose a dependency on react-addons-css-transition-group. Since this is in a require statement, would it ever try to require even if the user doesn't import React.AddOns.Animation.Transition? For example, if the code is bundled but not optimized.

I think it would be ideal if purescript-react only depended on the react NPM library and provided a PureScript interface solely for that. However, I am open to other thoughts or ideas on the subject. @paf31 what do you think?

[jasonzoladz]

I don't know the answer as to whether it's still require-d even when the user doesn't import the addons. (Though it'll be easy to check. I'll take a look later this evening.)

If it's not require-d, then I'd lean towards keeping it all together. Otherwise, I agree that it should be separated. And if it's separated, then you might consider an umbrella repo (a la purescript-argonaut) to encompass the react libraries.

[ethul]

I am digging the umbrella repo idea. I am just throwing this out there, but we could even make purescript-react-addons repo, which could be the umbrella for a number of react addons.

[jasonzoladz]

So, at least when using webpack, the add-ons are not require-d unless you import React.AddOns.Animation.Transition. I don't have a strong a opinion as to how the tree of libraries is structured. But I do think there should be one "root" where you can view the entire purescript-react ecosystem. (I think dispersion discourages beginners.)

[paf31]

I like the idea of an umbrella repo, but it would be important to keep things in sync. I'm fine with moving this to a separate repo, or having it here if we think it's more likely to be updated that way.

[ethul]

My vote is for creating separate repos for the addons and then creating an umbrella repo to make an all-in-one package. One main reason I am leaning toward this approach is because I did a quick test with psc-bundle (without any entry point modules specified) and running the resulting bundle threw an error that the NPM module couldn't be found. I don't know how often this case would come up, but I don't think we should require users to add an NPM dependency when they aren't using the library (in such a case).

I propose that we either create a purescript-react-addons repository where we can make bindings for react addons as we go. Or create a new repository for each addon; e.g., purescript-react-addons-css-transition-group. Then we can create an umbrella repo purescript-react-with-addons (or something). Also, I wonder if it makes sense to create an org purescript-react where all of these repos can go, like we have for purescript-node. If we want to go the org route, I'd be happy to set that up. But let me know what you both think.

In terms of the PR, I had a few comments I wanted to open up for discussion:

• Comment 1 - Should we use Date.Time.Milliseconds here?
• Comment 2 - Instead of redefining unsafeFromPropsArray, can we use the one defined at React.DOM.Props.unsafeFromPropsArray?
• Comment 3 - Is there a way to use React.createElement? Also, is there a difference passing the children as an array to createElement vs. spreading them as arguments? I see in the docs that a key props must always be used, so maybe they work the same way in this case?
• Comment 4 - This is sort of a minor point, but what do you think of putting everything into a module named React.Addons.CSSTransitionGroup? Just a quick thought.

[jasonzoladz]

Given that psc-bundle might throw an error (in the absence of an entry point), I agree that we should have separate repos under an umbrella; with separate repos for each add-on (as the npm modules are distinct).

Re: Comment 1: I didn't use Date.Time.Milliseconds because I didn't want to add another dependency. But I'd leave that choice up to you.

Re: Comment 2: See comment 3 concerning mkDOM.

Re: Comment 3: I considered using createElement, but decided against because ReactCSSTransition group doesn't fit into the mkDOM scheme. That is, you must pass the class instance itself -- not a string name like other vanilla elements (e.g., ul). So if we use createElement we still need a foreign import of the ReactCSSTransitionGroup itself, and then users must make the element themselves.

Re: Comment 4: I chose Transition because I haven't yet wrapped the lower-level ReactTransitionGroup API, and I think it belongs in the same module.

[jasonzoladz]

Ultimately, I don't have strong opinions on any of this stuff. :) My interest is only in having the library wrap as much of React as possible, since we are using it in production.

[ethul]

Thanks for the info.

Regarding Comment 1, I would not be opposed to using the Milliseconds type from Date.Time. It might be convenient. For Comment 2 and 3, I was wondering if something like the following might work.

module Foo (reactCSSTransitionGroup) where

import Prelude ((<<<))

import React (ReactClass(), ReactElement(), createElementDynamic)
import React.DOM.Props (Props(), unsafeFromPropsArray)

reactCSSTransitionGroup :: Array Props -> Array ReactElement -> ReactElement
reactCSSTransitionGroup = createElementDynamic reactCSSTransitionGroupClass <<< unsafeFromPropsArray

foreign import reactCSSTransitionGroupClass :: forall props. ReactClass props

And then in the FFI:

// module Foo

exports.reactCSSTransitionGroupClass = require('react-addons-css-transition-group');

Comment 4: Makes sense. The module as you have it works. Perhaps my only suggest would maybe to go with the prefix React.Addons.

Thank you for all of your work on this. I suppose it would make sense to either create a repo purescript-react-addons or purescript-react-addons-css-transition-group. Starting out I think either can work. @paf31 Do you have a preference one way or another?

[jasonzoladz]

That'll work. I'll make the changes this afternoon.

[ethul]

Great. Thanks!

On Saturday, 19 March 2016, jasonzoladz notifications@github.com wrote:

That'll work. I'll make the changes this afternoon.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#73 (comment)

[jasonzoladz]

Thanks for edits. It's better. Updated and rebased. (I decided to ditch Milliseconds and just use an Int. The marshalling didn't seem worth it here.)

[paf31]

Would you like me to create a repo in contrib then? Or did you want to start a new organization? I'm fine with either.

This all looks good to me 👍

[ethul]

I am just wondering if the require statement for react is necessary here?

[ethul]

The changes look good to me. I just had one comment above. Thanks for making these updates @jasonzoladz.

@paf31 I propose that we create purescript-react-addons-css-transition-group in purescript-contrib, and if we get more addons we can bring everything into a purescript-react org. Also, we could make purescript-react-with-addons as an umbrella repo. Does this work for everyone? I'd be open to starting out with the org, but I am not sure it is necessary just yet.

[jasonzoladz]

Unused import removed.

I'd vote to start with the org, or move to it pretty soon, as we have purescript-react-dom as well. Alternatively, you might include a couple lines and links on the purescript-react README that explains where everything is.

[ethul]

Sorry about the delay here. Maybe to get things rolling we can create a repo in purescript-contrib and then I can add links in the purescript-react README for each relevant repo as you suggest.

@paf31 Can you please create purescript-contrib/purescript-react-addons-css-transition-group?

@jasonzoladz Once we have this set up, can you please push the contents of this PR there?

I hope this sounds good to everyone.

I do think having an org purescript-react is the way to go at some point (soon), but maybe for now the above will work.

[jasonzoladz]

Sounds good. Will do.

[paf31]

Here you go, thanks.

[ethul]

Thanks!

[jasonzoladz]

Pushed to new repo.

Eric and Phil, thanks for all your work on this stuff. It's fantastic to be able to avoid TypeScript and its ubiquitous <any>.

[ethul]

Great! Thank you as well for putting all of this together.