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

markup using yaml syntax highlighting #19807

Merged
merged 1 commit into from Aug 22, 2022

Conversation

hamishwillee
Copy link
Collaborator

YAML code syntax highlighting is now supported on MDN: mdn/yari#6831

The only usage of it I could find was on the page structure docs. This adds it there.

@hamishwillee hamishwillee requested a review from a team as a code owner August 22, 2022 01:25
@hamishwillee hamishwillee requested review from dipikabh and removed request for a team August 22, 2022 01:25
@github-actions github-actions bot added the Content:Other Any docs not covered by another "Content:" label label Aug 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2022

Preview URLs

Flaws

URL: /en-US/docs/MDN/Writing_guidelines/Page_structures/Page_types/API_reference_page_template
Title: API reference page template
on GitHub
Flaw count: 10

  • macros:
    • /en-us/docs/web/api/mdn (url: /en-US/docs/Web/API/MDN) does not exist
    • /en-US/docs/Web/API/NameOfTheInterface/NameOfTheInterface does not exist
    • /en-US/docs/Web/API/NameOfTheInterface does not exist
    • /en-US/docs/Web/API/NameOfParentInterface does not exist
    • /en-US/docs/Web/API/NameOfTheInterface/property1 does not exist
    • and 4 more flaws omitted
  • bad_bcd_queries:
    • No BCD data for query: path.to.feature.NameOfTheInterface

External URLs

URL: /en-US/docs/MDN/Writing_guidelines/Page_structures/Page_types/API_reference_page_template
Title: API reference page template
on GitHub

No new external URLs

(this comment was updated 2022-08-22 23:00:05.229040)

@@ -18,7 +18,7 @@ browser-compat: path.to.feature.NameOfTheConstructor
> The frontmatter at the top of the page is used to define "page metadata".
> The values should be updated appropriately for the constructor.
>
> ```md
> ```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Actually this is Markdown. YAML is supported in Markdown as an embedded language. I learned that here: PrismJS/prism#3283 If md doesn't work it's probably because we listed yaml after md in the Prism loading list, and we just need to fix that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Josh-Cena

  1. I'd take an argument that this should be marked up as "md" because what we're showing here is the text to copy which is in "markdown" - as opposed to trying to show yaml for the sake of yaml. So close if you like.
  2. That said, marking up as yaml works and shows your clear intent. I'd much rather than in the normal case than marking up as markdown. If there needs to be a change to the tool to support this via markdown then cool, but an alias should still be added for yaml because I don't think using md for this case helps anyone.
  3. There wasn't any other easily discoverable yaml in the docs, which makes this whole conversation take up more time than it was worth :-).

Upshot, close this if you like. I don't plan to ever use md for "real" yaml though, or chase up changes in yari.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have strong opinions either way. We don't show much YAML/Markdown in actual content anyway, so it's not crucial for the sake of tooling. Front matter is definitely part of Markdown, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should keep this set as md, as it would provide better syntax highlighting and make it clearer to readers and tooling alike that the codeblock is Markdown. I feel this scenario is the same reason why we decided to add PowerShell and similar to the syntax highlighting, so that we aren't representing something as a language it is not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these blocks are not YAML because they include the front matter delimiters --- which are not part of YAML. So this:

---
title: NameOfTheConstructor()
slug: Web/API/NameOfTheParentInterface/NameOfTheParentInterface
page-type: web-api-constructor
tags:
  - API
  - Constructor
  - Reference
  - Experimental
  - Deprecated
  - Non-standard
browser-compat: path.to.feature.NameOfTheConstructor
---

...is not valid YAML, although this:

title: NameOfTheConstructor()
slug: Web/API/NameOfTheParentInterface/NameOfTheParentInterface
page-type: web-api-constructor
tags:
  - API
  - Constructor
  - Reference
  - Experimental
  - Deprecated
  - Non-standard
browser-compat: path.to.feature.NameOfTheConstructor

...is.

So I think we should close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

@wbamberg --- is part of YAML. It delimits documents. (A piece of YAML source can have multiple documents)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

^^^ FWIW that doesn't change that what we're showing in this particular case is markdown that happens to also be yaml. So I'm happy with the reversion.

@hamishwillee
Copy link
Collaborator Author

I have reverted all the "yaml" to "md" and changed the one case of "plain" to MD. This should be able to merge now.

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

The existing code blocks marked with md already seem to work, so this LGTM

@hamishwillee hamishwillee merged commit fd8b08b into mdn:main Aug 22, 2022
@hamishwillee hamishwillee deleted the yaml_highlighting branch August 22, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Other Any docs not covered by another "Content:" label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants