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

WIP: feat: adds frag snippet #738

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wavilikhin
Copy link

@wavilikhin wavilikhin commented Dec 15, 2024

fixes #735

🚧 This PR is not ready yet

Resolves to React.Fragment shorthand <></>

@sergeche
Copy link
Member

This PR requires additional improvements:

  • You should check the output with nested elements, like frag>ul>litems*4. Note that the output should be properly formatted
  • Also check how “Wrap with abbreviation” action works if user wants to wrap existing contents with frag snippet (see unit tests for example)

@wavilikhin
Copy link
Author

@sergeche Hey! I didn't see a CONTRIBUTING.md guide (I could help create one if needed) so I created this branch and marked it as WIP to show that some work is in progress. Its defineitely not ready yet.

Thanks for the comments! I'll expand the test coverage to cover all possible cases for sure. I'm still in the process of understanding what's going on here in general. For now, I've created a separate test/frag.ts file where all the test cases are located.

I've already added the support for frag and it wraps the passed content(in last commit). The only problem I have now is that it adds unnecessary <></> (I understand why, just searching for a better solution).

Quick question: since <></> is not a valid HTML tag, what should the snippet provide if the jsx.enabled: true option is not set? Should it just return an empty string, or should it work regardless of that option?

@@ -82,6 +82,14 @@ function element(node: AbbreviationNode, index: number, items: AbbreviationNode[
pushString(out, `</${name}>`);
commentNodeAfter(node, state);
}
} else if (config.options['jsx.enabled'] && !node.name) {
// Handle JSX fragment
Copy link
Author

Choose a reason for hiding this comment

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

That defineitely should be tweaked, but Im not sure yet if its a valid way to handle jsx fragment in general

Copy link
Member

Choose a reason for hiding this comment

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

No, nodes without name could be a (group), e.g. (ul>.item*4)+footer

const config: UserConfig = { options: { "jsx.enabled": true } };

it("should expand fragment", () => {
// should it work without explicit jsx.enabled ? It's not a valid html tag afaik
Copy link
Author

Choose a reason for hiding this comment

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

Here is the question i had. Should the result actually be expanded to <></> ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s OK to expand to <>\n\t${0}\n</> since its use case is to insert multiple nodes which will be formatted anyways

// should it work without explicit jsx.enabled ? It's not a valid html tag afaik
equal(expand("frag"), "<></>");
// invalid result, adds one more fragment
equal(expand("frag", config), "<></>");
Copy link
Author

Choose a reason for hiding this comment

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

TODO: actual result is invalid (<><></></>)

// invalid result, adds one more fragment at start
equal(
expand("frag>ul>li*4", config),
"<><ul>\n\t<li></li>\n\t<li></li>\n\t<li></li>\n\t<li></li>\n</ul></>"
Copy link
Author

Choose a reason for hiding this comment

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

TODO: actual result is invalid (<><></><ul>\n\t<li></li>\n\t<li></li>\n\t<li></li>\n\t<li></li>\n</ul></>)

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.

Support empty JSX fragments <></>
2 participants