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

Simplify markdown rendering for preferences #10589

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 3, 2022

What it does

Instead of using a custom MarkdownRenderer (introduced with #10431), we simplify the code by correctly using the markdownit library.

How to test

The same testing steps as in #10431, confirm that no regression has occurred.

Review checklist

Reminder for reviewers

@msujew
Copy link
Member Author

msujew commented Jan 3, 2022

cc @colin-grant-work You were right, there was a simpler way of handling things. I removed the MarkdownRenderer class. Hopefully, not too many downstream-users started depending on it.

@msujew msujew added the preferences issues related to preferences label Jan 3, 2022
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This approach is leaner than the previous one, and I observe no regressions:

  • Clicking on a preference name scrolls that preference into view
  • Clicking on a link opens the link in a new browser window.

@msujew msujew force-pushed the msujew/simplify-preference-rendering branch from 70c20c7 to cd49994 Compare January 5, 2022 16:08
@msujew
Copy link
Member Author

msujew commented Jan 5, 2022

@colin-grant-work FYI, added the removal of the MarkdownRenderer as a breaking change to the changelog. Will merge after CI is green.

@msujew msujew merged commit ff1121f into master Jan 5, 2022
@msujew msujew deleted the msujew/simplify-preference-rendering branch January 5, 2022 16:52
@vince-fugnitto vince-fugnitto added this to the 1.22.0 milestone Jan 27, 2022
@kittaakos
Copy link
Contributor

@msujew, I have noted that multiple markdown-it instances are used in the preference editor. markdow-it will require linkify-it, which will have the following regex for each preference value. Could Theia (re)use one markdown-it instance for parsing the markdown? With TheiaBlueprint Version 1.32.0 (1.32.0.92), I have dozens of the same regex string. Thank you!

linkify-it.mp4

@colin-grant-work
Copy link
Contributor

With the addition of markdown rendering in the Monaco package, I think it should be possible to get rid of markdown-it entirely in preferences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants