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

fix: no markdown support for underline #2072 #2073

Merged
merged 5 commits into from
Jun 18, 2020
Merged

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Jun 17, 2020

fix: #2072
Add support for underline in markdown by using ++underline++.
This fix will also show up in the editor

@auto-assign auto-assign bot requested a review from NGPixel June 17, 2020 08:08
@NGPixel
Copy link
Member

NGPixel commented Jun 18, 2020

2 things to improve:

  • Make it optional by adding a toggle as part of the Markdown Core module. Since this is not compliant with the CommonMark specification, it shouldn't be mandatory.

  • Avoid adding a new npm dependency for just 10 lines of code. Include the code directly.

@regevbr
Copy link
Contributor Author

regevbr commented Jun 18, 2020

Thanks @NGPixel for the comments.

I didn't use the library I mentioned in the issue as it will conflict with the current behavior of _sdf_ so I used a different one - markdown-it-plugin-underline which I can't locate its repository but it is a bit more complicated and contains 120 lines of code. So I don't think in-lining it will make sense.

About the toggle, I will add it

@regevbr
Copy link
Contributor Author

regevbr commented Jun 18, 2020

Done, can you please review?

@NGPixel
Copy link
Member

NGPixel commented Jun 18, 2020

Just not a fan of including an npm module with no public source available.

I think it would be more convenient and intuitive to use the _word_ syntax, which the other module does.

@regevbr
Copy link
Contributor Author

regevbr commented Jun 18, 2020

Ok, but itwill collide with current behavior causing change of ui for existing pages. If you are ok with it, I will be happy to do it

@NGPixel
Copy link
Member

NGPixel commented Jun 18, 2020

That's fine. It's an optional feature that can be turned off and almost everyone is using the *word* syntax for italic already. _word_ is simply more intuitive as underline for everybody.

@regevbr
Copy link
Contributor Author

regevbr commented Jun 18, 2020

@NGPixel I fixed it like you suggested, but I found myself duplicating the plugin code in both server and client. Is there a better way for it? (using the npm package is one way but you said you prefer not to)

@NGPixel
Copy link
Member

NGPixel commented Jun 18, 2020

Yeah it's not ideal for now, but the goal is to use the server pipeline in the future, eliminating the need to duplicate everything on the client.

@NGPixel NGPixel merged commit e03a80d into requarks:master Jun 18, 2020
@regevbr regevbr deleted the #2072 branch June 18, 2020 22:37
@regevbr
Copy link
Contributor Author

regevbr commented Jun 18, 2020

Thanks for the review and merge!

jionggyu pushed a commit to jionggyu/wiki-2.5.302-patch that referenced this pull request Jul 9, 2024
* fix: no markdown support for underline requarks#2072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: no markdown support for underline
2 participants