search

gakimball / react-inky

🦑 React components for Inky
https://gakimball.github.io/react-inky
MIT License
21 stars 10 forks source link

Migrate React.PropTypes to prop-types #9

Closed carlosvini closed 7 years ago

carlosvini commented 7 years ago

Result from running: jscodeshift -t react-codemod/transforms/React-PropTypes-to-prop-types.js react-inky/ See: https://facebook.github.io/react/blog/2017/04/07/react-v15.5.0.html#migrating-from-react.proptypes

carlosvini commented 7 years ago

I reverted the <tbody> commits and fixed the import order of React.

I understand it's important to keep it compatible with Inky library's test suite, but AFAIK ignoring these warnings might cause problems during updates.

Quote from https://github.com/facebook/react/issues/5652:

Browsers need the <tbody> tag. If it is not in your code, then the browser will automatically insert it. This will work fine on first render, but when the table gets updated, then the DOM tree is different from what React expects. This can give strange bugs, therefore React warns you to insert the <tbody>. It is a really helpful warning.

Inky has tbody in other parts of their templates, so it doesn't seem that tbody is something unsupported in email clients. Anyway, it's your choice.

gakimball commented 7 years ago

I didn't write the CSS for Foundation for Emails, so I can't speak to exactly why some components don't use <tbody>, but there's likely a reason. If it can be added without breaking anything in any of the two dozen email clients that F4E, then it should be proposed on the main F4E repo. (Although F4E is unfortunately stagnant right now. I don't think anyone is actively working on it.)

One more thing:

This will work fine on first render, but when the table gets updated, then the DOM tree is different from what React expects. This can give strange bugs

This thankfully doesn't affect react-inky, because you're only going to render React to HTML once—there's no subsequent re-rendering of components.

carlosvini commented 7 years ago

It's unfortunate to know F4E is stagnant.

I understand your reasons not to change tbody and I have already reverted these changes.

Could you guide me what's needed to merge this request, or if you decide not to merge it, can I close it?

Thx!

gakimball commented 7 years ago

Since the majority of the features in this PR aren't going through, could you put together a new branch with just the commits for the stuff we're keeping? Having the revert commits in the final PR is unnecessary.

The simplest way to pull out just the stuff we want into a new branch might be using git-cherry-pick.