Skip to content

Conversation

@remarkablemark
Copy link
Owner

@remarkablemark remarkablemark commented Jul 3, 2019

Relates to remarkablemark/html-react-parser#107

Would love to get your review @talbet

Save to `package.json` devDependencies
The build script is similar to HTML DOM config except SVG does not
have boolean or overloaded boolean attributes/properties.
And add "main" field in `package.json` so the file is included
during npm publish.
Copy link

@talbet talbet left a comment

Choose a reason for hiding this comment

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

Really nice. You must have a lot of familiarity with the React internals, I've looked at them a bit, but wouldn't have come up with this. A few comments, but nothing I can see that would block this from going in.

try {
fs.mkdirSync(LIB_DIR);
} catch (error) {
// throw error;
Copy link

Choose a reason for hiding this comment

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

Should the error be re-thrown here if it is not a "directory already exists" error?

if (error.code !== 'EEXIST') throw error;

Copy link
Owner Author

Choose a reason for hiding this comment

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

}

/**
* HTML DOM property config.
Copy link

Choose a reason for hiding this comment

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

Maybe we can add extra context to this comment? I wasn't sure what it was for at first. Something along the lines of: "an object containing HTML attributes and how they should be mapped to React props"

Copy link
Owner Author

Choose a reason for hiding this comment

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

42999e2

I changed the wording slightly. Let me know if it makes sense.

Copy link

Choose a reason for hiding this comment

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

Yeah, I think this makes it clearer.

try {
fs.mkdirSync(LIB_DIR);
} catch (error) {
// throw error;
Copy link

Choose a reason for hiding this comment

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

as above, we could handle this error if it is not expected

if (error.code !== 'EEXIST') throw error;

Copy link
Owner Author

Choose a reason for hiding this comment

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

var overloadedBooleanProperties = require('../../lib/html/overloaded-boolean-properties');

var attributeMap = {};
var attributeName;
Copy link

Choose a reason for hiding this comment

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

Nice 👍 I haven't used var for such a long time I probably would have forgotten the scoping complications of var and loops.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks. I'm considering rewriting this to TypeScript but that can be an enhancement done later.

@remarkablemark remarkablemark merged commit 6425501 into master Jul 5, 2019
@remarkablemark remarkablemark deleted the feat/react-property branch July 5, 2019 02:05
@remarkablemark
Copy link
Owner Author

Released and published react-property@0.1.0:

# npm
npm i -S react-property@0.1.0

# yarn
yarn add react-property@0.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants