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(livesample): support nolint aliases #7149

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

OnkarRuikar
Copy link
Contributor

Summary

Related to #7017

Recently we introduced new aliases js-nolint, html-nolint, and css-nolint in order to avoid some code fences getting linted.
But current live sample code doesn't recognize the new language tags.

Problem

The live sample code collector doesn't recognize -nolint language aliases.

Solution

Widen the Cheerio CSS attribute style selector using wild card e.g. pre[class*="${part}"]

How did you test this change?

Use same code for a live sample but with -nolint added to all language code fences.

cc/ @caugner @teoli2003

@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Sep 14, 2022
@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Sep 14, 2022

Only one selector out of all three, .js, pre[class*="brush: js"], or pre[class*="js"] is sufficient to achieve what we want.
Or if we want to be more specific then we can use two separate selectors pre[class*="brush: js"] and pre[class*="brush: js-nolint"].


Note: In exiting code two selectors do not work, only the first selector .${part} is effective at the moment.

  1. pre[class*="brush:${part}"]
    This doesn't have space between brush: and ${part}. The correct form is pre[class*="brush: ${part}"].

  2. pre[class*="${part};"]
    This doesn't work because of the ; after ${part}. The correct form is pre[class*="${part}"].

I may be missing some historical context.

@caugner
Copy link
Contributor

caugner commented Sep 14, 2022

@OnkarRuikar Can you give an example page that uses -nolint and a live sample?

@OnkarRuikar
Copy link
Contributor Author

@OnkarRuikar Can you give an example page that uses -nolint and a live sample?

We haven't tagged any livesample code with js-nolint, html-nolint, or css-nolint yet cos yari doesn't support it. We are skipping them to avoid breaking the output.

For example this sample. We want to use the nolint alias on html block cos Prettier is completing the <p> tag.
The code we want to use would be:

```html-nolint
<p>In HTML, you define a paragraph using the <p> element.</p>

<p>In HTML, you define a paragraph using the &lt;p&gt; element.</p>
```

In the live output below, you can see that the first paragraph has gone wrong. The browser interprets the second instance of `<p>` as starting a new paragraph. The second paragraph looks fine because it has angle brackets with character references.

{{ EmbedLiveSample('Entity_references_Including_special_characters_in_HTML', 700, 200, "", "") }}

@caugner
Copy link
Contributor

caugner commented Sep 14, 2022

@OnkarRuikar Could we try tackling this earlier during markdown-to-html conversion (markdown/m2h)?

diff --git a/markdown/m2h/handlers/code.ts b/markdown/m2h/handlers/code.ts
index c12eee6ee..921e22dae 100644
--- a/markdown/m2h/handlers/code.ts
+++ b/markdown/m2h/handlers/code.ts
@@ -6,7 +6,7 @@ import u from "unist-builder";
  */
 export function code(h, node) {
   var value = node.value ? node.value + "\n" : "";
-  const lang = node.lang;
+  const lang = node.lang.replace(/-nolint$/, '');
   const meta = (node.meta || "").split(" ");
   const props: { className?: string | string[] } = {};

@github-actions github-actions bot added markdown markdown related issues and pull requests and removed macros tracking issues related to kumascript macros labels Sep 15, 2022
@OnkarRuikar
Copy link
Contributor Author

Could we try tackling this earlier during markdown-to-html conversion (markdown/m2h)?

@caugner good point!
It is safe to do it because this is the only place in the entire repo where .lang is used. And it is better than fixing at many places.
I've tested and committed the change.


There is one possible, may be good, side effect. It will make yari oblivious to existence of -nolint.
Thus following code will become redundant:

["css-nolint", "css"],
["html-nolint", "html"],
["js-nolint", "js"],

@caugner caugner merged commit cce98ca into mdn:main Sep 15, 2022
@caugner
Copy link
Contributor

caugner commented Sep 15, 2022

There is one possible, may be good, side effect. It will make yari oblivious to existence of -nolint.

Thank you, and good catch, feel free to open a follow-up PR (though I won't be able to review it before Monday)! 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants