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 Markdown rendering within summary/detail #2059

Merged
merged 14 commits into from
Aug 8, 2019

Conversation

axic
Copy link
Member

@axic axic commented May 21, 2019

Tries to fix the rendering of https://eips.ethereum.org/EIPS/eip-1474

Idea from: gettalong/kramdown#155 (comment)

Fixes #1878.

@axic axic requested review from Arachnid and nicksavers May 21, 2019 17:51
@axic
Copy link
Member Author

axic commented May 21, 2019

This actually triggered two ERCs which have bad formatting:

- ./_site/EIPS/eip-1154.html
  *  171:14: ERROR: Unexpected end tag : dd (line 171)
  *  172:12: ERROR: Unexpected end tag : dd (line 172)
  *  173:10: ERROR: Unexpected end tag : dd (line 173)
  *  174:8: ERROR: Unexpected end tag : dd (line 174)
- ./_site/EIPS/eip-1620.html
  *  linking to internal hash #log-confirm-update that does not exist (line 228)
     <a href="#log-confirm-update">LogConfirmUpdate</a>
  *  linking to internal hash #log-create that does not exist (line 195)
     <a href="#log-create">LogCreate</a>
  *  linking to internal hash #log-execute-update that does not exist (line 230)
     <a href="#log-execute-update">LogExecuteUpdate</a>
  *  linking to internal hash #log-redeem that does not exist (line 217)
     <a href="#log-redeem">LogRedeem</a>
  *  linking to internal hash #log-revoke-update that does not exist (line 241)
     <a href="#log-revoke-update">LogRevokeUpdate</a>
  *  linking to internal hash #log-withdraw that does not exist (line 206)
     <a href="#log-withdraw">LogWithdraw</a>

@axic
Copy link
Member Author

axic commented May 23, 2019

I've fixed some of these "issues" locally, but actually realised that for some reason (probably due to Gemfile.lock) the github-pages packages is stuck at 179, while the current release 198. I'm sure the new version could fix a lot of rendering problems as it brings in newer kramdown.

@Arachnid
Copy link
Contributor

Do you want to pursue this PR further?

@axic
Copy link
Member Author

axic commented May 26, 2019

Yes. Need to figure out how I can convince my bundler to update the versions.

@axic axic force-pushed the jekyll-markdown branch 3 times, most recently from 013d9be to 62c17a5 Compare July 2, 2019 02:30
@axic axic force-pushed the jekyll-markdown branch from 7e8e2f0 to 9963540 Compare July 19, 2019 18:25
@eip-automerger
Copy link

eip-automerger commented Jul 19, 2019

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

# This is the default, but be explicit as some EIPs depend on it
auto_ids: true
# This is to ensure more determistic behaviour
auto_id_stripping: true
Copy link
Member Author

@axic axic Jul 19, 2019

Choose a reason for hiding this comment

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

Link generation is explained here: https://kramdown.gettalong.org/converter/html.html#auto-ids

Newer version of kramdown changed the rules, hence the changes to existing EIPs. The htmlproofer check verifies that the links are pointing to existing headers.

@@ -36,6 +36,12 @@ twitter:
# Build settings
markdown: kramdown
theme: minima
kramdown:
parse_block_html: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Turned this to true means that markdown parsing is enabled within <details> blocks. However it is not working well. The closing HTML tags are not parsed.

This introduces problems with other EIPs which use the details block properly (820, 1820, etc.): only with a larger code snippet and not with embedded markdown.

The resolution is to change EIP-1474 instead, which was the culprit triggering this entire PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can update EIP-1474 accordingly.

Copy link
Member Author

@axic axic Jul 19, 2019

Choose a reason for hiding this comment

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

@bitpshr thanks! I already did in this PR.

@axic
Copy link
Member Author

axic commented Jul 19, 2019

@Arachnid should be ready to merge.

@@ -1397,8 +1393,6 @@ The `name` field defines which compiler was used in compilation.
</table>


<div id="version-version-1"></div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Kramdown generates proper links out the headers now, no need for these. If left here htmlproofer complains about duplicate IDs. Manually verified they point to the right header.

@axic
Copy link
Member Author

axic commented Jul 26, 2019

@Arachnid any comments on merging this?

@axic
Copy link
Member Author

axic commented Aug 8, 2019

@Arachnid @nicksavers any objections merging this?

@Arachnid Arachnid merged commit bba07b8 into ethereum:master Aug 8, 2019
@axic axic deleted the jekyll-markdown branch August 8, 2019 22:17
ilanolkies pushed a commit to ilanolkies/EIPs that referenced this pull request Nov 12, 2019
* Fix Markdown rendering within summary/detail

* Add vendor to .gitignore

* Remove duplicate github-pages entry from Gemfile

* Require github-pages 198

This brings in fixes to kramdown.

* Remove explicit jekyll version as github-pages brings it in as a dependency

* Update bundler dependency tree

* Fake bundler version

* use Ruby 2.3.0

* Set sane defaults for kramdown

* Fix links after kramdown update

* Remove <details> formatting from EIP-1474 as it is not working with embedded markdown

* Revert "Fix Markdown rendering within summary/detail"

* Fix email in EIP-1812

* Remove <details> formatting from EIP-1620 as it is not working with embedded markdown
MadeofTin pushed a commit to MadeofTin/EIPs that referenced this pull request Nov 13, 2019
* Fix Markdown rendering within summary/detail

* Add vendor to .gitignore

* Remove duplicate github-pages entry from Gemfile

* Require github-pages 198

This brings in fixes to kramdown.

* Remove explicit jekyll version as github-pages brings it in as a dependency

* Update bundler dependency tree

* Fake bundler version

* use Ruby 2.3.0

* Set sane defaults for kramdown

* Fix links after kramdown update

* Remove <details> formatting from EIP-1474 as it is not working with embedded markdown

* Revert "Fix Markdown rendering within summary/detail"

* Fix email in EIP-1812

* Remove <details> formatting from EIP-1620 as it is not working with embedded markdown
BelfordZ pushed a commit to BelfordZ/EIPs that referenced this pull request Dec 13, 2019
* Fix Markdown rendering within summary/detail

* Add vendor to .gitignore

* Remove duplicate github-pages entry from Gemfile

* Require github-pages 198

This brings in fixes to kramdown.

* Remove explicit jekyll version as github-pages brings it in as a dependency

* Update bundler dependency tree

* Fake bundler version

* use Ruby 2.3.0

* Set sane defaults for kramdown

* Fix links after kramdown update

* Remove <details> formatting from EIP-1474 as it is not working with embedded markdown

* Revert "Fix Markdown rendering within summary/detail"

* Fix email in EIP-1812

* Remove <details> formatting from EIP-1620 as it is not working with embedded markdown
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* Fix Markdown rendering within summary/detail

* Add vendor to .gitignore

* Remove duplicate github-pages entry from Gemfile

* Require github-pages 198

This brings in fixes to kramdown.

* Remove explicit jekyll version as github-pages brings it in as a dependency

* Update bundler dependency tree

* Fake bundler version

* use Ruby 2.3.0

* Set sane defaults for kramdown

* Fix links after kramdown update

* Remove <details> formatting from EIP-1474 as it is not working with embedded markdown

* Revert "Fix Markdown rendering within summary/detail"

* Fix email in EIP-1812

* Remove <details> formatting from EIP-1620 as it is not working with embedded markdown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EIP1474 Does not render correctly on main EIPs website
6 participants