Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Adding mixin with animation support #41

Merged
merged 16 commits into from
May 10, 2016
Merged

Adding mixin with animation support #41

merged 16 commits into from
May 10, 2016

Conversation

krawaller
Copy link
Contributor

I couldn't help myself, so this PR implements the mixin suggested in #40. In essence it gives us animation support for whatever DOM mutating library you can use with React-Faux-DOM. As an added bonus it makes static usage easier too!

I also threw in an example (the chart from the blog post rewritten to use the mixin) and updated the docs.

@Olical
Copy link
Owner

Olical commented Mar 4, 2016

Looks pretty slick, nice one! 👍

I'll go through this in detail soon, it's a big change so I'll make sure I'm happy with the approach and can't see any issues. It looks like it'll be the best middleground though. An approach which didn't require ANY changes to the D3 code would be the ideal situation I guess, but may not be possible.

The only thing that's smelling to me is specifying these timeouts, it'd be awesome if we didn't have to. I'll go through it line by line soon! :)

Great work, very happy to see this sort of contribution, I was hoping someone would come by with a smart solution like this and solve animations for me, I didn't feel comfortable enough with vanilla D3 animations to make the call. You certainly delivered!

@krawaller
Copy link
Contributor Author

With regards to no changes to the D3 code, I'd say we're as close as we're gonna get. Mike's code would work as is if we put the radio button and chart as children to a faux parent element and then fed the chart one to D3, but i couldn't be bothered. :)

I agree specifying timeouts is a bit icky, but any other approach would be lib-specific. This is the only way (that I can see) to make an agnostic solution. Of course, having this solution doesn't prevent adding more convenient library-specific hooks further down the road!

Definitely take your time, i felt a bit intrusive re: the scope of this PR. No pride invested, so feel totally free to shoot it down or change stuff around.

@krawaller
Copy link
Contributor Author

Another note; this mixin, or a similar setup, would likely serve as a good base for future library-specific code.

For instance, the D3 hook from my earlier chart that we used like this:

rect.transition()
    .attr('y', function (d) { return y(d.y0 + d.y) })
    .attr('height', function (d) { return y(d.y0) - y(d.y0 + d.y) })
    .call(hook)

...would be very easy to (re)implement using .animateFauxDOM(Infinity) and .stopAnimateFauxDOM().

@@ -1 +1,2 @@
node_modules/
bundle.js
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May as well move this to examples/animate-d3-with-mixin/.gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes a ton of more sense. Good catch, will do.


It's great for...
React-faux-DOM supports a wide range of DOM operations and will fool most libraries but it isn't exhaustive (the full DOM API is ludicrously large). It supports enough to work with D3 but will require you to fork and add to the project if you encounter something that's missing.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: If it's hyphenated keep it all lowercase and usually wrapped in backticks (for a code block). In this context I would write it as "ReactFauxDOM" or just "The faux DOM..."

@Olical
Copy link
Owner

Olical commented May 9, 2016

Sorry for leaving this sitting around for so long. I was planning on merging and releasing this since it doesn't change existing functionality. Are you okay with that? It looks good to me!

@krawaller
Copy link
Contributor Author

Of course, go right ahead!

@Olical Olical merged commit d810416 into Olical:master May 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants