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

Generate Table of Contents for each chapter #232

Merged
merged 11 commits into from
Oct 29, 2019
Merged

Generate Table of Contents for each chapter #232

merged 11 commits into from
Oct 29, 2019

Conversation

mikegeyser
Copy link
Contributor

This generates a table of contents from the output of the html generation, with links to individual sections. It is currently completely unstyled, so that we can use the work already done as a part of #199.

The code below is based on #222, so at present it looks a lot larger than it is, and that PR needs to be merged first. (I've kept them separate, so that they're easier to review - I hope that's ok.)

Please let me know what you think, and I'll make whatever changes are necessary.

@mikegeyser mikegeyser added the development Building the Almanac tech stack label Oct 24, 2019
@mikegeyser mikegeyser added this to the SHIP IT! milestone Oct 24, 2019
@mikegeyser mikegeyser requested review from tyohan and rviscomi October 24, 2019 12:02
@mikegeyser mikegeyser self-assigned this Oct 24, 2019
@rviscomi
Copy link
Member

Getting this error when running locally:

 Generating chapter: en, 2019, performance
(node:9069) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'children' of undefined
    at nest_headings (/Users/rviscomi/git/google/almanac.httparchive.org/src/tools/generate/generate_table_of_contents.js:61:37)
    at nest_headings (/Users/rviscomi/git/google/almanac.httparchive.org/src/tools/generate/generate_table_of_contents.js:60:39)
    at generate_table_of_contents (/Users/rviscomi/git/google/almanac.httparchive.org/src/tools/generate/generate_table_of_contents.js:9:27)
    at parse_file (/Users/rviscomi/git/google/almanac.httparchive.org/src/tools/generate/generate_chapters.js:28:15)
    at generate_chapters (/Users/rviscomi/git/google/almanac.httparchive.org/src/tools/generate/generate_chapters.js:18:45)
    at <anonymous>
(node:9069) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 3)
(node:9069) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@mikegeyser
Copy link
Contributor Author

I've spotted the error, going to need to rework something.

@rviscomi rviscomi changed the title Table of Contents Generate Table of Contents for each chapter Oct 24, 2019
@tyohan
Copy link
Contributor

tyohan commented Oct 25, 2019

I got some error on generating chapters locally

Traceback (most recent call last):
  File "generate_chapters.py", line 70, in <module>
    generate_chapters()
  File "generate_chapters.py", line 24, in generate_chapters
    write_template(language, year, chapter, metadata, body)
  File "generate_chapters.py", line 58, in write_template
    file_to_write.write(template.format(body=body, metadata=metadata))
ValueError: expected ':' after conversion specifier```

@tyohan
Copy link
Contributor

tyohan commented Oct 25, 2019

@mikegeyser i saw you're generated the toc on the client side with JS. I've done this with mistune so the chapter templates will have toc in generated templates. Also, trying to make sure the CSS styles will shared with methodology pages.

This is how Javascript chapter looks like in chapter-page branch
image
And in mobile
image

Sorry i reused your avatar @rviscomi for Addy :D, need to see how the page looks like.

I can finish this chapter page by today so we can see how the chapter pages can be generated config file and markdown contents. So no need for extra json or other files. WDYT @rviscomi

@mikegeyser mikegeyser mentioned this pull request Oct 25, 2019
@mikegeyser
Copy link
Contributor Author

@tyohan The table of contents is being statically generated, not on the client side.

I agree regarding the CSS, I deliberately haven't added styling to this because of the other work that you were doing. :)

@rviscomi
Copy link
Member

Sorry we have two very similar PRs doing similar things so there's some confusion. Let's try to divide the work up:

@tyohan focus on styling the Chapter page. For now you can create a static "lorem ipsum" table of contents for styling.

@mikegeyser continue working on the showdown migration and TOC generation.

Does that work for everyone?

@tyohan
Copy link
Contributor

tyohan commented Oct 27, 2019

@rviscomi @mikegeyser looks good for me. Will continue to work on on styling

@rviscomi
Copy link
Member

@mikegeyser are these changes working and ready for review?

@rviscomi rviscomi added the ASAP This issue is blocking progress label Oct 29, 2019
@mikegeyser
Copy link
Contributor Author

@rviscomi I believe it's working, will rebase off of master and test again. The only outstanding change is to move the html generation into a template, which I will also do now.

@mikegeyser
Copy link
Contributor Author

Sorry for the churn, I believe that this is ready to go now. I have some more that I want to add (formatting and linting, to pick up errors more easily) but I'll do it in a separate PR.

@mikegeyser mikegeyser mentioned this pull request Oct 29, 2019
Copy link
Contributor

@tyohan tyohan left a comment

Choose a reason for hiding this comment

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

I've test this one with my current styling from #251 but need to tune a bit and work perfectly. Good one!

@rviscomi
Copy link
Member

@mikegeyser I've merged #251 which created a merge conflict in chapter.html. I was going to try to resolve it but I'll leave that to you since it depends on how you want to implement the TOC markup.

@mikegeyser
Copy link
Contributor Author

mikegeyser commented Oct 29, 2019

@rviscomi @tyohan I've merged master into this PR and resolved the conflict. It looks right to me, but could you take a look and see if it's right?

The Author information and the Previous/Next isn't implemented yet, should that happen in this PR or can it be in a follow up? (It's not strictly speaking related to this.)

Sorry, I just read this update from @tyohan. Please belay the last, it's been a long day... :P

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@rviscomi rviscomi merged commit 05862f7 into master Oct 29, 2019
@rviscomi rviscomi deleted the toc branch October 29, 2019 20:46
@rviscomi rviscomi removed the ASAP This issue is blocking progress label Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Building the Almanac tech stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants