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.

Sign up for GitHub

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

Jump to bottom

refactor: migrate to goldmark #89

Merged
merged 5 commits into from
Feb 9, 2023
Merged

refactor: migrate to goldmark #89

merged 5 commits into from
Feb 9, 2023

Conversation

unknwon
Copy link
Member

@unknwon unknwon commented Feb 9, 2023 •
edited
Loading

Migrate our Markdown render from blackfriday to the state-of-the-art goldmark, which gives us more familiarity with GitHub GFM.

This refactoring should remain feature parity.

Test plan

Inline anchor CleanShot 2023-02-09 at 17 07 37@2x
TOC and Markdown function (jsonschema) CleanShot 2023-02-09 at 17 08 16@2x
Search CleanShot 2023-02-09 at 17 10 06@2x
Warning and note CleanShot 2023-02-09 at 17 10 47@2x

@unknwon unknwon mentioned this pull request Feb 9, 2023
Closed
@unknwon unknwon marked this pull request as ready for review February 9, 2023 09:02
@unknwon unknwon requested review from mrnugget and sqs February 9, 2023 09:02
sqs
sqs approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@sqs sqs left a comment

Choose a reason for hiding this comment

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

Awesome, this is so great! Can you try it out on docs.sourcegraph.com locally and see if there are any regressions? (I mean, not 100% strict inspection, but just doing a quick check.)

@unknwon
Copy link
Member Author

unknwon commented Feb 9, 2023 •
edited
Loading

Can you try it out on docs.sourcegraph.com locally and see if there are any regressions? (I mean, not 100% strict inspection, but just doing a quick check.)

This is what I've been testing with locally, I'll post some notable elements in PR description 😁 Added!

mrnugget
mrnugget approved these changes Feb 9, 2023
View reviewed changes
Copy link

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Have to admit: didn't review all of it and also not super carefully. If you tested this with our existing docs and the most complex pages look fine (something like this: https://docs.sourcegraph.com/batch_changes/how-tos/configuring_credentials) - let's do this!

@unknwon
Copy link
Member Author

unknwon commented Feb 9, 2023

CleanShot 2023-02-09 at 17 24 39@2x

@unknwon
Copy link
Member Author

unknwon commented Feb 9, 2023

Given we don't publish new versions automatically by just merging to main, I think it is super low-risk to merge this and iterate from here!

sashaostrikov
sashaostrikov approved these changes Feb 9, 2023
View reviewed changes
Copy link

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

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

NICE!

@unknwon unknwon merged commit da8faf4 into main Feb 9, 2023
@unknwon unknwon deleted the jc/migrate-to-goldmark-2023 branch February 9, 2023 09:32
@sqs sqs added the hackathon label Feb 9, 2023
This was referenced Feb 9, 2023
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqs sqs sqs approved these changes

@mrnugget mrnugget mrnugget approved these changes

@sashaostrikov sashaostrikov sashaostrikov approved these changes

Assignees
No one assigned
Labels
hackathon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unknwon @sqs @mrnugget @sashaostrikov