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

Consider moving to CommonMark #282

Open
2 tasks
jamietanna opened this issue Nov 11, 2021 · 1 comment
Open
2 tasks

Consider moving to CommonMark #282

jamietanna opened this issue Nov 11, 2021 · 1 comment

Comments

@jamietanna
Copy link
Contributor

Sounds like a plan re using our version 👍

That's fair - I think if it were me, I'd potentially be using this OpenAPI description with different tools (i.e. it may go onto the API Catalogue or other internal/external tooling, such as rendering inside tools like JIRA, or an IDE) and I'd be expecting a consistent formatting across all of those.

It's unlikely that many users will be affected, but the reason CommonMark became a thing is because so many Markdown flavours are inconsistent, so it's possible that people will be affected by quirks that RedCarpet has.

I agree that it'd be odd to write a doc page that doesn't render the same as the API docs, and to that I wonder if maybe we should look at using CommonMark everywhere?

Originally posted by @jamietanna in #281 (comment)

Notes:

  • GitHub previews won't necessarily match the site's view of content, so may make it less easy for folks just reviewing through GitHub
  • Would require a SemVer major bump, as it would introduce chance of breakage, unless made opt-in
jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 12, 2021
@jamietanna
Fix: Don't break TOC when OpenAPI description includes headers
5b738ab 
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by Middleman
- but as it's possible for an OpenAPI description to include headers, we
won't always have one.

Until the `openapi3_parser` gem supports making this configurable, which
is unlikely, due to the way that CommonMark doesn't have this as a
default extension, we can instead utilise our `TechDocsHTMLRenderer` to
render the headings with IDs, as expected.

To do this, we can create a new helper in `ApiReferenceRenderer` which
will render the Markdown document as our custom Markdown, which requires
we construct the `TechDocsHTMLRenderer` class, which needs the Middleman
`ConfigContext` class injected in.

This means that we won't be using CommonMark, as per the OpenAPI spec,
and this is something we'll look to address in alphagov#282.

[0]: alphagov/tdt-documentation#156
@jamietanna jamietanna mentioned this issue Nov 12, 2021
Merged
jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 12, 2021
@jamietanna
Fix: Render OpenAPI Markdown with RedCarpet
29362c9 
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by the
`TechDocsHTMLRenderer` - but as we've been rendering the markdown
through `openapi3_parser`'s CommonMark implementation, these IDs are not
being populated.

Until the `openapi3_parser` gem supports making this configurable, which
is unlikely, due to the way that CommonMark doesn't have this as a
default extension, we can instead utilise our `TechDocsHTMLRenderer` to
render the headings with IDs, as expected.

To do this, we can create a new helper in `ApiReferenceRenderer` which
will render the Markdown document as our custom Markdown, which requires
we construct the `TechDocsHTMLRenderer` class, which needs the Middleman
`ConfigContext` class injected in.

As well as the original `info.description` that we'd planned to amend,
we can also replace any other CommonMark->HTML rendering at the same
time.

This means that we won't be using CommonMark, as per the OpenAPI spec,
and this is something we'll look to address in alphagov#282.

The setup for our tests are now a little more complex, as we're not able
to (easily) inject `RedCarpet`, so need to mock a bit more to make it
work.

[0]: alphagov/tdt-documentation#156
@lfdebrux
Copy link
Member

This would be useful for the Learn to Code repo, which has an issue because Redacarpet doesn't support Markdown in HTML in the same way CommonMark does:

alphagov/learn-to-code#56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Technical Documentation Template project board
Status: Backlog
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lfdebrux @jamietanna