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

Update language-highlight.md #1251

Merged
merged 2 commits into from
Jun 27, 2020
Merged

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Jun 27, 2020

Summary

Add some details regarding syntax highlight.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

@trusktr trusktr added the docs related to the documentation of docsify itself label Jun 27, 2020
@vercel
Copy link

vercel bot commented Jun 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/fvu66nrrm
✅ Preview: https://docsify-preview-git-update-language-highlight-docs.docsify-core.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9f3cf1e:

Sandbox Source
weathered-fog-bgi1m Configuration

@sy-records sy-records merged commit 76050e3 into master Jun 27, 2020
@sy-records sy-records deleted the update-language-highlight-docs branch June 27, 2020 10:05
@@ -8,4 +8,25 @@
<script src="//cdn.jsdelivr.net/npm/prismjs/components/prism-php.min.js"></script>
```

To use the new languages, make sure the code block label matches the part after `prism-` in the file name. FOr example, for `prism-bash.js` write a code block labeled with `bash` like this:
Copy link
Member

Choose a reason for hiding this comment

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

Minor typo: "FOr" should be "For" (lower-case "O").

```
````

?> Note that with GitHub-flavored markdown, `sh` and `bash` are effectively aliases of each other, but this is not the case with Prism. So using `sh` will not enable `bash` syntax in this case.
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing for a few reasons:

  1. GitHub-flavored markdown doesn't handle syntax highlighting (see spec). I think you're referring to how GitHub handles markdown.
  2. Docsify supports GitHub-flavored markdown (see the Supported Markdown specifications for marked)
  3. Prism's source lists sh as an alias for bash.
  4. Prism's docs list shell as an alias for bash (and sh is an alias for shell).

I suggest we just delete the ?> Note... section.

Copy link
Member Author

Choose a reason for hiding this comment

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

GitHub-flavored markdown doesn't handle syntax highlighting

Maybe I should say "Markdown on GitHub".

Copy link
Member Author

Choose a reason for hiding this comment

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

Prism's source lists sh as an alias for bash.

When I add prism-bash.js and use sh as the syntax it doesn't highlight. Same with shell. bash works though. Is it new? Does it work for you? Maybe I need an update.

Copy link
Member

Choose a reason for hiding this comment

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

I was incorrect about sh being an alias for shell and bash in the Prism source (it was late 🤷 ). Prism simply associates sh as a file extension for bash scripts.

The difference between GitHub and Prism highlighting is:

  • Prism requires using the alias(es) listed in their docs. For Bash, Prism lists only bash and shell. I can confirm both bash and shell work as expected in docsify-themeable, while sh does not work:

    Screen Shot 2020-06-28 at 11 24 09 PM
  • GitHub (it appears) uses file extensions (like sh) in addition to the usual aliases. Notice how the same markdown from the screenshot above renders here on GitHub (specifically, the proper sh highlighting):

    # bash
    echo "hello"
    # shell
    echo "hello"
    # sh
    echo "hello"

    To confirm this, I tried to find another file type that Prism associates an extension with but does not list as an alias. Batch has a file extension of bat in the source but only lists batch as an alias in the docs so this is a good match. Here's how batch and bat render on docsify-themeable:

    Screen Shot 2020-06-29 at 12 20 17 AM

    And here's how that above markdown renders here on GitHub:

    :: batch
    @ECHO "HELLO"
    :: bat
    @ECHO "HELLO"

Summary: Docsify uses Prism aliases only, GitHub uses language aliases and extensions.


?> Note that with GitHub-flavored markdown, `sh` and `bash` are effectively aliases of each other, but this is not the case with Prism. So using `sh` will not enable `bash` syntax in this case.

For `prism-php.js`, it would be:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need two examples? If we want two, why not use one of the built-in languages (css, html, or js) and one that requires loading an additional language?

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Jun 27, 2020

@sy-records I realize this has already been merged, but there are a few tweaks in here worth making.

Also, the "At least 2 approving reviews are required to merge this pull request" rule generally means "two reviews not including your own if you are the person who submitted the PR".

@sy-records
Copy link
Member

Sorry, This was PR by @trusktr submitted, I modified a PHP syntax error.

I reviewed it with @Koooooo-7 ...😅

@trusktr Can you resubmit a PR to fix these problems?

@sy-records
Copy link
Member

Oh no, why is it submitted to the master branch? ? ?😱

@jhildenbiddle
Copy link
Member

Ahh. Sorry, @sy-records. I saw your edit and thought it was your PR.

@Koooooo-7
Copy link
Member

@jhildenbiddle sorry for missing checking the details of documentation.

@sy-records , we need revert this PR from master.

@sy-records
Copy link
Member

Let's submit another PR. @Koooooo-7 @trusktr

@jhildenbiddle
Copy link
Member

@Koooooo-7 -- No worries at all. That's what multiple reviewers are for. 😄

@trusktr
Copy link
Member Author

trusktr commented Jun 29, 2020

Whoops, I didn't check what branch it merged to when I opened it. I think by default GitHub sets the target branch to whatever the default branch is (should be develop). I'm not on the comp, I'll check later.

Sure, I can also submit updates.

@trusktr
Copy link
Member Author

trusktr commented Jun 29, 2020

A general rule that I believe we should follow: Unless the PR author is unresponsive, let's let the PR author merge the PR after it is approved, and not the reviewers. The author might have ideas or want to change something, or similar.

@jhildenbiddle
Copy link
Member

@trusktr -- How about something like this:


Language highlighting

Docsify uses Prism to highlight code blocks in your pages. Prism supports the following languages by default:

  • Markup - markup, html, xml, svg, mathml, ssml, atom, rss
  • CSS - css
  • C-like - clike
  • JavaScript - javascript, js

Support for additional languages is available by loading the language-specific grammar files via CDN:

<script src="//cdn.jsdelivr.net/npm/prismjs@1/components/prism-bash.min.js"></script>
<script src="//cdn.jsdelivr.net/npm/prismjs@1/components/prism-php.min.js"></script>

To enable syntax highlighting, wrap each code block in triple backticks with the language specified on the first line:

```html
<p>This is a paragraph</p>
<a href="//docsify.js.org/">Docsify</a>
```

```bash
echo "hello"
```

```php
function getAdder(int $x): int 
{
    return 123;
}
```

The above markdown will be rendered as:

<p>This is a paragraph</p>
<a href="//docsify.js.org/">Docsify</a>
echo "hello"
function getAdder(int $x): int 
{
    return 123;
}

@trusktr
Copy link
Member Author

trusktr commented Jul 2, 2020

That sounds great! I will hit accept if you copy/paste it to a PR.

@jhildenbiddle
Copy link
Member

@trusktr -- Will do. 😄

jhildenbiddle added a commit that referenced this pull request Jul 2, 2020
Proposed changes described in comment from #1251
@jhildenbiddle jhildenbiddle mentioned this pull request Jul 2, 2020
19 tasks
sy-records added a commit to sy-records/docsify that referenced this pull request Jul 12, 2020
* Update language-highlight.md

* Update language-highlight.md

Co-authored-by: 沈唁 <52o@qq52o.cn>
sy-records pushed a commit to sy-records/docsify that referenced this pull request Jul 12, 2020
Proposed changes described in comment from docsifyjs#1251
@sy-records sy-records mentioned this pull request Jul 12, 2020
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs related to the documentation of docsify itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants