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(docs): Use Kramdown processor #6677

Merged

Conversation

charlottetan
Copy link
Contributor

@charlottetan charlottetan commented Jan 23, 2022

What does this PR do?

Related to PR #5810. Fixes #6508.
Using GFM causes issues with anchor tags, using Kramdown would solve the issues with anchor tags, but results in issues with links, see discussion in kramdown/parser-gfm#28. So the link was manually added, i.e., using [url](url) instead of just url.

Checklist:

  • Read our contributing guidelines
  • Search for duplicates.
  • Include author(s) and platform where appropriate.
  • Put lists in alphabetical order, correct spacing.
  • Add needed indications (PDF, access notes, under construction)

Follow-up

  • Check the status of GitHub Actions and resolve any reported warnings!

Initial addressed at commit 83b7ee4
_config.yml Show resolved Hide resolved
@davorpa
Copy link
Member

davorpa commented Jan 23, 2022

NOTES

Apply this PR first than #6625 to resolve conflicts there.

ABOUT HELP WANTED...

To compare results between GitHub Preview (GH) and GitHub Pages + Jekyll (GHP), post here your hosted fork at github.io/[user]/free-programming-books making the GHP settings point to your PR branch

We need mount versioned environments using different config parameters:

  • markdown: GFM
  • markdown: kramdown
  • markdown: kramdown + kramdown.input: GFM (default)
  • markdown: kramdown + kramdown.input: commonmark (this is more compatible with remark, the library we use back fpb-linter although it not fully compatible with GH and GHP)
  • ...

For more info see docs at:

@charlottetan
Copy link
Contributor Author

Nice catch ❤️ . It's fine remove those bunch of trailing spaces.

There are two more. I'll fix it with moving all links refs definitions to bottom

Thanks! Removing the trailing spaces was actually required for the parsing, otherwise it wouldn't render right

@abhishekpandeykr

This comment has been minimized.

@charlottetan
Copy link
Contributor Author

charlottetan commented Jan 24, 2022

  • markdown: GMF
  • markdown: kramdown
  • markdown: kramdown + kramdown.input: GMF (default)
  • markdown: kramdown + kramdown.input: commonmark (this is more compatible with remark, the library we use back fpb-linter although it not fully compatible with GH and GHP)

Typo here, you probably meant GFM. We don't need to try the first two options -

  • markdown: GFM

This is what the main repo has today, which works except for the issues with rendering anchors that have special characters (#6508)

  • markdown: kramdown

This is the option in this PR/branch, I've added it back in to make it explicit but it doesn't need to be there. This works for both the RTL text parsing and the anchor rendering, but requires that plain URLs be marked up too, i.e., written as [url](url).

So we only need to test these two:

  • markdown: kramdown + kramdown.input: GFM
  • markdown: kramdown + kramdown.input: commonmark

Not sure what the goal is here, you'd like to test them so that we can avoid having to change the URLs? What should be tested when changing the different processor configurations?

specifying `markdown: kramdown` explicitly did not change url parsing behavior
@charlottetan
Copy link
Contributor Author

I tested these two other configurations:

markdown: kramdown + kramdown.input: GFM

PR #6679 - this renders the same specifying just markdown: kramdown in PR #6677.
Which also renders the same as not specifying any markdown processor at all and using the default.

markdown: kramdown + kramdown.input: commonmark

PR #6680 - this results in a build error because the commonmark processor is not supported:
github-pages 223 | Error: kramdown has no parser to handle the specified input format: commonmark

@charlottetan charlottetan marked this pull request as ready for review January 24, 2022 06:27
@charlottetan
Copy link
Contributor Author

charlottetan commented Jan 26, 2022

Here are some other discussions about linking:
barryclark/jekyll-now#459 (comment)
kramdown/parser-gfm#17

@eshellman eshellman merged commit de2f319 into EbookFoundation:main Jan 31, 2022
@davorpa
Copy link
Member

davorpa commented Jan 31, 2022

Previewing I realize now that #6662 is broken.

image

To solve, it needs add markdown="1" attribute to <div> too

<div align="center">
[![Awesome](https://cdn.rawgit.com/sindresorhus/awesome/d7305f38d29fed78fa85652e3a63e154dd8e8829/media/badge.svg)](https://github.com/sindresorhus/awesome)
[![License: CC BY 4.0](https://img.shields.io/badge/License-CC%20BY%204.0-lightgrey.svg)](https://creativecommons.org/licenses/by/4.0/)
</div>

@EbookFoundation EbookFoundation locked as resolved and limited conversation to collaborators Jan 31, 2022
@davorpa davorpa added the 🐛 BUG Confirmed bugs, normally at GitHub Pages label Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 BUG Confirmed bugs, normally at GitHub Pages help wanted Needs help solving a blocked / stucked item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The 0-MOOC and Algorithms and Data Structures anchor tags not working
4 participants