Skip to content
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

Add support for JSX fragments #25

Merged
merged 4 commits into from
Sep 8, 2018

Conversation

reyronald
Copy link
Contributor

Closes #18

Now that Babel 7 was officially released a few days ago, I think we can add fragments support here.

How do you feel about it? Do you think we should say something different in the README? How about the implementation?

@fregante
Copy link
Collaborator

fregante commented Sep 5, 2018

If you want to fix the build error, you can drop '4' from .travis.yml as a consequence of this xojs/xo@d1eb47c#diff-354f30a63fb0907d4ad57269548329e3

@fregante
Copy link
Collaborator

fregante commented Sep 5, 2018

Actually I fixed the tests in #26 so we can probably release this and #22 as v4

I haven't tested this PR but it looks good. Have you tried it in your projects?

@reyronald
Copy link
Contributor Author

Great, thanks for the fix! I was gonna do it today but you beat me to it.

I haven't tried it in my projects yet to be honest, but I wrote the new tests taking into account how I would use it and what I would expect from it. Do you spot anything that I could've missed, or any particular manual test that would be healthy to perform?

readme.md Outdated
pragmaFrag: "DocumentFragment",
}
]
];

Choose a reason for hiding this comment

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

Use single-quotes

readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@@ -78,6 +81,7 @@ const build = (tagName, attrs, children) => {
};

function h(tagName, attrs) {
// eslint-disable-next-line prefer-rest-params

Choose a reason for hiding this comment

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

Why are you disabling this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent was to keep the original source code as intact as possible, so I decided the disable the rule for the line below instead of changing it. I have no problem in fixing it though, if you prefer :)

Choose a reason for hiding this comment

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

No, what you have is ok. Was just wondering whether there was another reason you disabled it.

Copy link
Contributor Author

@reyronald reyronald left a comment

Choose a reason for hiding this comment

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

I also added ticks and a link to pragma as well to keep it consistent, let me know if that's ok.

@@ -78,6 +81,7 @@ const build = (tagName, attrs, children) => {
};

function h(tagName, attrs) {
// eslint-disable-next-line prefer-rest-params
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent was to keep the original source code as intact as possible, so I decided the disable the rule for the line below instead of changing it. I have no problem in fixing it though, if you prefer :)

@vadimdemedes
Copy link
Owner

Thanks @reyronald, great PR!

@vadimdemedes vadimdemedes merged commit a7e4406 into vadimdemedes:master Sep 8, 2018
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.

4 participants