-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
jsx #133
base: master
Are you sure you want to change the base?
Conversation
For testing sample.tsxconst Yoo = () => {
return (
<div>
<section>
<p>hello</p>
<p
he="llo"
wor="ld"
attr={{
...window,
hello: () => {
return (
<section>
<p>IDK</p>
</section>
);
},
}}
>
hello
</p>
<p>{true ? "true" : "false"}</p>
<p>{true && "true"}</p>
<p>
{true && (
<section>
<p>This is awesome</p>
</section>
)}
</p>
<div id="div">
{getUser({
name: "numToStr",
job: "making plugins",
})}
</div>
</section>
<div className="flex items-center justify-center image-uploader">
<span className="animate-ping h-4 w-4 rounded-full bg-sky-400 opacity-75"></span>
</div>
</div>
);
}; |
0fe68bd
to
dd60ffa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Excellent! Waiting for this 🥂 |
@kuntau It would be helpful if you can test this out and report any cases where comments are not correct apart from the issues that are already listed :) |
Previously mentioned issues are now fixed (workaround) so this is now usable (I am using it) but there might be some cases where different commentstring might get used. I am leaving this PR for further testing. To be clear I am not aiming for 100% accuracy but ~90%. So, there is a chance that some issue won't get fixed. |
28cd517
to
7a284b9
Compare
I was trying this out, it seems that this doesn't trigger for .tsx files? Works great for .js files though, though it seems that it comments individual lines rather than a block. Is that the expected behaviour? CleanShot.2022-05-31.at.08.42.28.mp4 |
Works for me though. Do you have tsx parser installed?
I think you are pressing |
I wasn't aware that there's a Everything seems to work as expected, including |
@ecosse3 @ShiChenCong Nice catch! I'll try to fix those issue. |
@ShiChenCong Is that a self closing element in the image? Can you reply with the element that you are facing issue with, if that's ok. |
@numToStr I've been using this PR for the last few days and have not noticed any weird behavior whatsoever. Seems to be working quite great in a React environment for me so far! Thanks for the hard work on this 😄 |
I can confirm the bugs that @ecosse3 has reported. It sometimes can't comment regular HTML tags correctly. for some reason if I remove the 2022-06-04.11-38-27.mp4Also: 2022-06-04.11-43-01.mp4The jsx: <NodeViewWrapper className="flex items-center justify-center image-uploader">
{loading && (
<span className="animate-ping h-4 w-4 rounded-full bg-sky-400 opacity-75"></span>
)}
</NodeViewWrapper> |
@ahhshm I was able to reproduce the same. My observation is that any element that has attributes on the same line alongside the opening element is using wrong comment string. It doesn't matter whether it's a custom or native elements. |
@numToStr Sorry for reply so late, yes, It is self closing element |
@ShiChenCong No problem. Now it should work with self closing element. |
@ahhshm @ecosse3 I pushed a fix that should resolve the issue that you were facing before. |
@numToStr Sorry for the late response. It seems like updates have solved all of the previous problems. I just found a small bug, it has a little problem with uncommenting opening fragment tags: 2022-06-18.12-08-23.mp4with ts_context_commentstring: 2022-06-18.12-16-33.mp4The jsx (for testing): <NodeViewWrapper as="figure" dir="ltr" data-type="custom-image">
{node.attrs.src && (
<>
<img
src={node.attrs.src}
alt="Image"
className={`${node.attrs.src.startsWith("data:") && "blur-sm"}`}
/>
<bdi>
<NodeViewContent as="figcaption" className="text-center" />
</bdi>
</>
)}
</NodeViewWrapper> |
@ahhshm hmmm...this also seems to be broken in context-commentstring. I'll try to fix this. |
3171b6a
to
ba36b4b
Compare
Any update on this? I've been using this branch since it was created and it's working fine for me. I haven't encountered any specific bug in a while and although it's a WIP, it seems pretty stable. Anything that I can help you with to merge it into the main beach sooner? |
@ahhshm I am really happy to hear that this is working without any major hiccups. As I said earlier, my main reason for holding this out is because
Both would've helped in making the implementation robust and much nicer but it seems both PRs are kinda blocked, sadly. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Update: Updating the pre_hook now shows the nil error everywhere, not just in JSX files. Please help. |
This is also happening to me. |
Any news about this? I am getting |
tl;dr - I am just playing with the idea of making
jsx
first-class, probably viapre_hook
but if the implementation gets more complicated than I hope it to be then I'll probably make this a separate plugin.Usage
Why?
context-commentstring
.jsx
is the only thing that is missing to make this plugin a masterpiece.What's missing?
In its current state, it works but does have some issues regarding accuracy
1. Can't comment/uncomment multiple attributes at once 1Fixed using workaroundDetails
(Also when uncommenting, expecting
//
to be used instead of{/* */}
)2. Can't uncomment the following at once 1Fixed using workaroundDetails
(This is not a priority, as the syntax is already broken)
FAQ
Because everyone is not using
jsx
so if you need it then that's da way.As I said it depends on the implementation. If it's small, simple, and covers most cases then I'll merge it.
I don't use
jsx
that much, so if you find something feel free to comment. And to be clear I am not aiming for 100% accuracy but ~90%. So, there is a chance that some issue won't get fixed.Yes, you can definitely use it. And as you know JS is JS so this will always remain experimental.
Footnotes
Both of them are a non-issue if you comment/uncomment them individually ↩ ↩2