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

Chapter page #236

Closed
wants to merge 5 commits into from
Closed

Chapter page #236

wants to merge 5 commits into from

Conversation

tyohan
Copy link
Contributor

@tyohan tyohan commented Oct 25, 2019

Notes on this PR:

  • chapter template design implementation ( still need some touch on details )
  • table of content automatically generated
  • not include permalink to headings( Add the ability to permalink all chapter headings and figures #199 ) but with the current implementation it will be easy to generate a permalink to each heading
  • authors box, the authors will automatically generated from config data. Will not shows up if the authors metadata can't found a match from contributors data in config
  • previous and next chapter button will automatically generated from config, the script will iterate through all chapters in config and will set previous and next button once it found the same chapter number from chapter metadata.
  • change the methodology page CSS file to page.css and reuse the CSS file with this chapter page

* added chapter template
* refactor methodology page to share CSS styles with chapter page
@tyohan tyohan requested review from mikegeyser and rviscomi October 25, 2019 07:25
@tyohan
Copy link
Contributor Author

tyohan commented Oct 25, 2019

Got some error after merge with master

  File "generate_chapters.py", line 160, in <module>
    generate_chapters()
  File "generate_chapters.py", line 76, in generate_chapters
    write_template(language, year, chapter, metadata, body, tochtml)
  File "generate_chapters.py", line 125, in write_template
    file_to_write.write(template.format(body=body, metadata=metadata, toc=tochtml, authors=authors, prevnext=prevnext))
ValueError: expected ':' after conversion specifier

Will try to fix this first

@tyohan
Copy link
Contributor Author

tyohan commented Oct 25, 2019

Ok, all good now. It should be safe.

@@ -1,26 +1,36 @@
{# IMPORTANT!
{{# IMPORTANT!
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @tyohan, the changes that were made to the chapter.html in #222 were so that we can use Node + Showdown + EJS to statically generate the templates - because mistune wasn't generating tables (amongst other potential problems).

If we add the extra curly braces back, it will break the ejs rendering of the markdown. The work done in #232 is supposed to try and help with this, but I'm not sure how best to proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikegeyser sorry accidentally click re-request review button.

Noted on showdown. Let me try to work around this thing and see if we can combine our works.

Copy link
Member

Choose a reason for hiding this comment

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

We should only maintain one markdown parsing script and given that mistune doesn't support markdown tables within HTML, we should deprecate this mistune version in favor of the showdown script @mikegeyser is building.

Sorry that you put so much work into this script and for any confusion as we migrated tools. Having only one script will simplify things a lot.

@tyohan tyohan requested a review from mikegeyser October 25, 2019 13:21
@tyohan
Copy link
Contributor Author

tyohan commented Oct 25, 2019

@rviscomi there are 2 ways to merge my code and @mikegeyser :

  1. Let showdown generate the body through Node but others generate with Python and mistune. We can use Naked to run Node from Python, so we just need to call it once from Python script.
  2. Move some of my code to Flash app route, so contributors and previous & next navigation will render there.

WDYT?

@@ -1,9 +1,44 @@
argh==0.26.2
Copy link
Member

Choose a reason for hiding this comment

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

Wow this is a lot of new requirements! Where did they all come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was installing the mistune contrib module and trying to update the the requirements but didn't realized it somehow generated all of these :|

@@ -0,0 +1,24 @@
{% for author in authors %}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than individual template files for each of these components, we can generate them in the existing chapter template file.

Similar to the render_byline macro in base_chapter.html.

@@ -0,0 +1,25 @@
<nav id="chapter-navigation">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to generate these as part of the markdown rendering. We have the chapter info in 2019.json and we can render them server-side.

Also we might not launch with all 20 chapters, so some are marked todo in the config. These links should go to the next/previous sequential chapter that isn't still marked "todo". That should be done dynamically on the server-side because it could change after the markdown is initially rendered.

@@ -0,0 +1,24 @@
{% for author in authors %}
Copy link
Member

Choose a reason for hiding this comment

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

No need to update Japanese templates, we can leave that for the final translation step.

@@ -1,26 +1,36 @@
{# IMPORTANT!
{{# IMPORTANT!
Copy link
Member

Choose a reason for hiding this comment

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

We should only maintain one markdown parsing script and given that mistune doesn't support markdown tables within HTML, we should deprecate this mistune version in favor of the showdown script @mikegeyser is building.

Sorry that you put so much work into this script and for any confusion as we migrated tools. Having only one script will simplify things a lot.

@rviscomi rviscomi added the development Building the Almanac tech stack label Oct 25, 2019
@rviscomi rviscomi added this to the Infrastructure complete milestone Oct 25, 2019
@rviscomi
Copy link
Member

@tyohan any progress on this PR? I think most of the TOC work can be reverted (sorry) and we can proceed with the markup/style changes.

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

tyohan commented Oct 29, 2019

Noted, i'll push in a few hours.

@tyohan
Copy link
Contributor Author

tyohan commented Oct 29, 2019

I have a new PR #251. Feel free to delete this branch.

@rviscomi rviscomi closed this Oct 29, 2019
@tunetheweb tunetheweb deleted the chapter-page branch November 27, 2019 11:45
@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