New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support custom elements in React 16 #78

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
3 participants
@bschlenk
Contributor

bschlenk commented Nov 30, 2018

Closes #77

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2018

Coverage Status

Coverage increased (+0.1%) to 99.291% when pulling 7b2c5a8 on bschlenk:support-custom-elements into bfe2023 on remarkablemark:master.

@remarkablemark

Thanks for creating the PR @bschlenk, I added some comments. Let me know what you think

@@ -15,8 +15,7 @@ DOMProperty.injection.injectDOMPropertyConfig(
* @param {Object} attributes - The attributes.
* @return {Object} - The props.
*/
function attributesToProps(attributes) {
attributes = attributes || {};
function attributesToProps(attributes = {}) {

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

revert: can we undo this line change since the library isn't transpiled and I wouldn't want the ES6 syntax to break for other users

/**
* Check if a given tag is a custom component.
*
* @see https://github.com/facebook/react/blob/master/packages/react-dom/src/shared/isCustomComponent.js

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

docs: could you use the following link instead: https://github.com/facebook/react/blob/v16.6.3/packages/react-dom/src/shared/isCustomComponent.js

This way, it's likely to 404 if it moves around

*/
function isCustomComponent(tagName, props) {
if (tagName.indexOf('-') === -1) {
return props && typeof props.is === 'string';

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

docs: do you mind adding a comment about the custom elements specification (https://www.w3.org/TR/2016/WD-custom-elements-20161013/#attr-is)

*
* @return {boolean}
*/
function reactSupportsUnknownAttributes() {

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

perf: can we memoize the value here to minimize execution cycles?

var PRESERVE_CUSTOM_ATTRS = React.version.split('.')[0] >= 16;
// JS will coerce the value to number due to the comparison
// http://www.ecma-international.org/ecma-262/5.1/#sec-11.8.5

refactor: can we rename the value as PRESERVE_CUSTOM_ATTRS (so it follows the convention of a constant) and add a comment to the blog post above the line?

The only disadvantage of this approach is that the value isn't readonly or a constant so it's possible for others to override it

const assert = require('assert');
const attributesToProps = require('../lib/attributes-to-props');
describe('attributesToProps', () => {
let actualReactVersion;

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

style: add newline

@@ -5,6 +5,15 @@ const domToReact = require('../lib/dom-to-react');
const { data, render } = require('./helpers/');
describe('dom-to-react parser', () => {
let actualReactVersion;

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

style: add newline

});
describe('utilities.reactSupportsUnknownAttributes', () => {
let actualVersion;

This comment has been minimized.

@remarkablemark

remarkablemark Dec 4, 2018

Owner

style: add newline

To not block this PR from getting merged, I'll make the fixes/changes on my side

@remarkablemark remarkablemark merged commit 9eeef55 into remarkablemark:master Dec 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 99.291%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment