-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Update documentation for auto-start and prism.ts #3885
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
base: v2
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dev-prismjs-com ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* prism.highlight('var foo = true;', 'javascript') | ||
* // Returns: | ||
* `<span class="token keyword">var</span> foo <span class="token operator">=</span> <span class="token boolean">true</span><span class="token punctuation">;</span>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense also to use modern JS in examples, like here, to use let
instead of var
?
* You obviously have to change this value before the automatic highlighting started. To do this, you can add an | ||
* empty Prism object into the global scope before loading the Prism script like this: | ||
* You have to change this value before the automatic highlighting starts. To do this, you can add an | ||
* empty Prism object into the global scope before the Prism script loads like this: | ||
* | ||
* ```js | ||
* window.Prism = window.Prism || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still rely on window
? Shouldn't we use globalThis
here or something?
* | ||
* ```js | ||
* window.Prism = window.Prism || {}; | ||
* Prism.manual = true; | ||
* // add a new <script> to load Prism's script | ||
* ``` | ||
* | ||
* or you can set `data-manual` on the script tag used to load PrismJs: | ||
* ```html | ||
* <script src="prism.js" data-manual></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like examples of that kind depend on what Prism would look like in v2. I thought it should become ESM-first, no?
So it should be either <script src="prism.js" type="module" data-manual></script>
or <script src="prism.mjs" data-manual></script>
. Or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can’t get a reference to the script element that triggered loading in ESM. It would need to be an attribute placed anywhere (eg data-prism-manual) or we’d key on the fil name or we’d also support non-ESM imports (by having an entry point that uses dynamic import to load everything else). Or all of the above 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good point, I'll try redoing the examples as an actual 3rd party project instead of doing it locally
@@ -162,7 +162,13 @@ export class Prism { | |||
* Usually a language definition like `Prism.languages.markup`. | |||
* @returns The highlighted HTML. | |||
* @example | |||
* Prism.highlight('var foo = true;', 'javascript'); | |||
* import { Prism } from './src/core/prism'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might miss some essential things, but shouldn't those imports be ”shorter“ (if I could say so)? Like, import Prism, { Token } from "prismjs"
, import javascript from "prismjs/languages"
. Is this something we should fix in package.json
? Do I understand correctly that we use these examples to show how to use Prism as an NPM package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a higher leve, these are meant to be small snippets, not full code examples. I think it’s ok if this doesn’t include the import code, which is different depending on tooling/env.
* import { Token } from './src/core'; | ||
* import { Prism } from './src/core/prism'; | ||
* import javascript from './src/languages/prism-javascript'; | ||
* | ||
* const prism = new Prism(); | ||
* prism.components.add(javascript); | ||
* | ||
* const tokens = prism.tokenize(`var foo = 0;`, prism.components.getLanguage('javascript')!); | ||
* tokens.forEach((token: Token | string) => { | ||
* if (token instanceof Token && token.type === 'number') { | ||
* console.log(`Found numeric literal: ${token.content}`); | ||
* } | ||
* }); | ||
* // Logs: Found numeric literal: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd mark it as a code block (```ts in this case). That way, TypeDoc will know this snippet should be highlighted appropriately.
* const prism = new Prism(); | ||
* prism.components.add(javascript); | ||
* | ||
* const tokens = prism.tokenize(`var foo = 0;`, prism.components.getLanguage('javascript')!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, why not single quotes here instead of ` and `?
* ```html | ||
* <script src="prism.js" data-manual></script> | ||
* ``` | ||
* | ||
* @default false | ||
* @public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove it. If I'm not mistaken, the property visibility is already described in the code (if not specified, it's public by default). TypeDoc also mentions that this tag is redundant (and not desirable). And the result TypeDoc produces is not that good:
* ```html | ||
* <script src="prism.js" data-manual></script> | ||
* ``` | ||
* | ||
* @default false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, we have options here. Either use @default
and get
Or we can use the TypeDoc @defaultValue
and get the result I find a bit more readable
* import { Prism } from './src/core/prism'; | ||
* import javascript from './src/languages/prism-javascript'; | ||
* const prism = new Prism(); | ||
* prism.components.add(javascript); | ||
* prism.highlight('var foo = true;', 'javascript') | ||
* // Returns: | ||
* `<span class="token keyword">var</span> foo <span class="token operator">=</span> <span class="token boolean">true</span><span class="token punctuation">;</span>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
* ```js
* import { Prism } from './src/core/prism';
* import javascript from './src/languages/prism-javascript';
* const prism = new Prism();
* prism.components.add(javascript);
* prism.highlight('var foo = true;', 'javascript')
* ```
* Returns:
* ```html
* <span class="token keyword">var</span> foo <span class="token operator">=</span> <span class="token boolean">true</span><span class="token punctuation">;</span>
* ```
produces a slightly better result
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, thanks for working on this! I left some comments, and I see Dmitry did too. The most important one I think is that code examples above functions don’t need to be complete with imports etc, especially since import code differs by environment (eg build tools or no build tools, JS vs TS, etc).
No description provided.