Support react 16 (quick way)

[ngryman]

This PR takes the fastest path to support React 16 with one breaking change: the domNode passed to the component is already mutated so referring to its textContent will not work anymore.

I've skipped the associated test for now and wait for opinions on how to handle this.

cc @jdeal @vitorbal
ref #10

[jdeal]

@ngryman Can you describe why the breaking change occurs?

[jdeal]

It seems like this just keeps using the unstable API and so avoids the breaking change?

[ngryman]

@jdeal Fixed the CI to highlight the issue: https://travis-ci.org/zapier/react-element-portal/builds/290528365#L1499

[jdeal]

I see now @ngryman. That's definitely a breaking change. I think we'll want to allow passing in a callback instead. So something like:

const Greeting = ({textContent}) => (<div>Hello {textContent}</div>);
const mapDomNodeToProps = (domNode) => ({
  textContent: domNode.textContent
});
render(
  <div>
    <ElementPortal selector="li.greeting" view={Greeting} mapDomNodeToProps={mapDomNodeToProps}/>
  </div>,
  document.getElementById(appId)
);  

That way we can call the callback before we render, similar to how we grab props from dataset. Effectively, dataset will become the preferred way to pass data, and mapDomNodeToProps will be a way to customize that.

[ngryman]

@jdeal Yeah I've thought about that and it's the better compromise I think. If we're doing so, why not having only this callback and deprecate the data attribute?

[jdeal]

@ngryman I'm okay with that, but I do feel like the data attribute is pretty handy here and feels like a good recommended approach for passing data. Of course, it's also wasted cycles if you have a lot of attributes you don't use. But I don't know if that's a huge concern here. I'm kinda torn. Maybe we add mapDomNodeToProps and hold off on the data decision for now? We could export a helper I guess for that purpose too. I wouldn't want to force the dataset boiler-plate out to userland.

[ngryman]

mapDomNodeToProps was what I was thinking, it gives lots of flexibility to the user while removing complexity our side.

I'm okay with that, but I do feel like the data attribute is pretty handy here and feels like a good recommended approach for passing data.

With the introduction of mapDomNodeToProps, the user now has a generic way of extracting data from the pre-existing target node and pass it to the transported component.
IMHO, keeping the data attribute would be redundant and opinionated here. If the user needs to parse data attributes, then he could use a utility package for that (https://github.com/rafaelrinaldi/data-attributes):

import dataAttributes from 'data-attributes'

const mapDomNodeToProps = (node) => ({
  data: dataAttributes(node)
})

<ElementPortal selector=".target" view={Component} mapDomNodeToProps={mapDomNodeToProps} />

[jdeal]

@ngryman I'm cool with that change, but can you replace the current data example in the README with your snippet using the data-attributes module? And we should probably publish this as 1.0.0?

[ngryman]

I've updated the docs, please correct/enhance my English :)

While patching zapier/zapier to check things work correctly (and it works btw 🎉), I think we could take the chance to change the API a little bit for something a little more concise. Nothing really fancy, just some tweaks that I'm gonna put in a PR tomorrow.

Meanwhile, if you think everything is ok for the React 16 migration, please tell me so I can reference it in zapier/zapier and ask for a review there.

[jdeal]

@ngryman A couple small wording tweaks, but otherwise we're good!

[jdeal]

Probably want something like:

One advantage of using the view prop is the ability to derive properties from the original DOM node and pass them to the the component.

[jdeal]

Small tweak here:

By using the mapDomNodeToProps prop, you can easily pass this data like so: