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

MDX compatible #53

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

Conversation

ApplebaumIan
Copy link

For my use case I'm using an Application Docusaurus, that converts markdown into ReactJS JSX. The issue with how this application originally works is that it uses HTML style comments which breaks React builds.
I'd like to propose using the markdown comment style

[//]: # ( readme: contributors -start )
[//]: # ( readme: contributors -end )

I have completely changed the current regex definition in JS in all areas I could find and it seems to work well for my use case, however to avoid breaking changes I could implement some sort of if statement to check the config file whether a user would like to use HTML or Markdown style comments.

@ApplebaumIan ApplebaumIan changed the title JSX compatible MDX compatible Aug 16, 2022
@akhilmhdh
Copy link
Owner

Hey @ApplebaumIan . Wow, first of all thank you so much for your kind efforts. Really appreciated.

Why i didn't went for this approach on markdown is that, actually markdown doesn't have a comment syntax as in an official way.

Ref: https://www.jamestharpe.com/markdown-comments/

Thus there is like n-ways. That's why went to html based commenting like other Github actions. But didn't know this would break markdown parsers, espectially mdx-parser.

Now coming back to the PR. I hope you didn't manually change the dist folder. Kindly run npm run build to generate dist.

And i think yah we do need fallback to html commenting also. Let me know if you can work on this further. If not i can take it furthur. Feel free to raise any doubts regarding this.

Would recommend actually moving to a file that contains functions that gives the html matching style and comment style based on the markdown flag

@ApplebaumIan
Copy link
Author

I can imagine! If I were writing this from scratch I probably would have chose HTML comments too for its ubiquity. I think I could take this on. Also I did in fact run npm run build I also performed all of the other linting, formatting, and testing!

@ApplebaumIan ApplebaumIan changed the title MDX compatible WIP MDX compatible Aug 16, 2022
@ApplebaumIan ApplebaumIan changed the title WIP MDX compatible 🚧 WIP MDX compatible Aug 16, 2022
@ApplebaumIan
Copy link
Author

I made the changes to the code making HTML the fall back! Just need to update the README.

@akhilmhdh
Copy link
Owner

akhilmhdh commented Aug 18, 2022

Hey @ApplebaumIan. Yup. Understood, just gave a heads up on generating the dist that's all. Now worries.

Great job on implementing this feature. But i do have one suggestion. Instead of having multiple files for mdx and other one.

Don't u think its better to separate just the regex and strings into seperate functions, into a file, instead of having multiple files

@akhilmhdh
Copy link
Owner

Btw i just now tried html commenting in md in Docusaurus. It was working fine 🤔

@ApplebaumIan
Copy link
Author

Really? Thats strange! I'm definitely doing something a little bit rogue. I'm using the README.md file as the front page for the site. Here's the project https://github.com/ApplebaumIan/tu-cis-4398-docs-template .

@ApplebaumIan
Copy link
Author

Hey @ApplebaumIan. Yup. Understood, just gave a heads up on generating the dist that's all. Now worries.

Great job on implementing this feature. But i do have one suggestion. Instead of having multiple files for mdx and other one.

Don't u think its better to separate just the regex and strings into seperate functions, into a file, instead of having multiple files

So instead of coreMdx.js add the functions to core.js. That would probably be a lot cleaner and less confusing to future contributors. I'll see If I can implement it!

@akhilmhdh
Copy link
Owner

So instead of coreMdx.js add the functions to core.js. That would probably be a lot cleaner and less confusing to future contributors. I'll see If I can implement it!

Yup. Will be much better. I'll be soon changing the entire codebase on v4 launch. Has a lot of things to consider before that though. A whole set of features coming soon.

@ApplebaumIan ApplebaumIan changed the title 🚧 WIP MDX compatible MDX compatible Aug 18, 2022
@ApplebaumIan
Copy link
Author

@akhilmhdh I believe this is ready for your review! :)

@ApplebaumIan
Copy link
Author

Btw i just now tried html commenting in md in Docusaurus. It was working fine 🤔

So I figured out what causes the issue... it's not what I thought at all. It turns out Docusaurus doesn't like that there is no newline between the comment and the table...

Screen Shot 2022-08-18 at 4 16 14 PM

When I add the newline it actually works as intended. 🤦‍♂️

Screen Shot 2022-08-18 at 4 17 13 PM

My immediate reaction was that it was the html comments because I'm so used to ReactJS and JSX breaking because comments aren't wrapped with {/* */}. Oh well. The solution still works and adds a different type of comment!

@akhilmhdh
Copy link
Owner

Hey @ApplebaumIan . Glad it's not breaking any more atleast 😅

But yah ill test this out and let you know. Just wanna make sure it's not breaking anything

@andresin87
Copy link

🆙

@akhilmhdh
Copy link
Owner

Hey @andresin87. I have been busy with my other works. Thus not able give attention to this project for sometime. I am working on v3 for this action but may take some more time.

Let me see when I get the time to review this. But mdx does work file u need to keep the indentation that's all as the previous comment suggested. So this is not blocking anything as i know.

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