Skip to content

Podcast: simplify newsletter references #1083

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

Merged
merged 4 commits into from
Apr 23, 2023

Conversation

kouloumos
Copy link
Contributor

This PR removes the need for boilerplate logic when adding newsletter references. The new logic requires just the podcast timestamps to be added in the related newsletter sections.

Current logic:

  • In the podcast, recreate the newsletter structure:
    • add title and anchor for each discussed item
    • add timestamp
    • add anchors for each segment in the transcript
  • In the referenced newsletter, for each item, add a link to the podcast page

New logic:

  • In the podcast:
    • point to the newsletter it references using the reference variable in the frontmatter
  • In the referenced newsletter:
    • add timestamp (podcast reference mark) for each discussed item

For each podcast, the recap_references_generator will parse the reference newsletter (declared in frontmatter) looking for podcast reference marks (timestamps) in order to create the newsletter references used by {% include newsletter-references.md %} in the podcast page.

@kouloumos kouloumos force-pushed the podcast-references branch 3 times, most recently from b5ce462 to fd58b3f Compare April 12, 2023 17:21
@harding
Copy link
Collaborator

harding commented Apr 13, 2023

Nice! This looks like it doesn't require any changes to the newsletter writing/editing flow, so concept ACK from me.

@kouloumos would you mind separating out the change that creates _plugins/common_utils.rb and calls it from _plugins/auto-anchor.rb into a separate commit? I'd like to verify that the change doesn't break any existing anchors and having that in a separate commit makes before/after testing easier.

This change will break existing anchors related to the podcast. I think we could probably significantly reduce the breakage by keeping the existing anchors by dropping their visual identifiers, or we could just bite the bullet and make this one-time change to a relatively new part of the site. I leave that decision to @bitschmidty and @xekyo.

The only other thing I plan to test is the impact on site build time.

@kouloumos kouloumos force-pushed the podcast-references branch from fd58b3f to c1c765e Compare April 14, 2023 09:57
@kouloumos
Copy link
Contributor Author

PR updated with @harding's commit separation request.

This change will break existing anchors related to the podcast. [...] or we could just bite the bullet and make this one-time change to a relatively new part of the site.

It's indeed a breaking change, but I saw it as acceptable because those anchors are mostly used/useful internally (linking/navigation between different sections of the website) and because it's a relatively new part of the site the external references to those anchors should be close to zero. Also, note that the affected anchors are only those at the transcript's segment titles as the ones at the beginning (of the page) were already based on the title of the list item and therefore are unaffected (with a couple of exceptions).

The only other thing I plan to test is the impact on site build time.

Build time on my machine is ~72s for this branch vs ~66s for master

Removes the need for boilerplate logic when adding newsletter references
The new logic requires just the podcast timestamps to be added in the
related newsletter sections.
For each podcast, the `recap_references_generator` will parse the
`reference` newsletter (declared in frontmatter) looking for podcast
reference marks (timestamps) in order to create the newsletter
references for the podcast page.
Copy link
Contributor

@bitschmidty bitschmidty left a comment

Choose a reason for hiding this comment

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

@kouloumos thank you for this PR! It will be nice to cutout the boilerplate pieces (and associated time!). Im glad you have the jekyll/rb chops to put this together.

ACK

Im pushing 245/246 pods to this thread and closing associated PRs then we can have everything in the new approach.

@bitschmidty
Copy link
Contributor

Retooled pod246 in the new format, which was very smooth, thanks @kouloumos and pushed a commit for it here

Rebased on master to get the latest newsletter editions and had to force push (sorry!)

If @harding is ok regarding any performance concerns, I think we would be ok to merge.

@harding
Copy link
Collaborator

harding commented Apr 22, 2023

ACK 9724a7a

Verified that a2cb309 doesn't break any existing anchors (thanks @kouloumos for splitting that commit out! For reference in case anyone else verifies this, I rearranged the order of commits for verification so that the function was only called by the legacy code)

Checked the build time; there was no meaningful difference here either way.

Previewed the site locally and clicked some random links to ensure that they worked as expected after the update.

I only quickly skimmed the added podcast content.

This is great, thanks!

@bitschmidty bitschmidty merged commit bf98147 into bitcoinops:master Apr 23, 2023
@bitschmidty
Copy link
Contributor

Thank you again to @kouloumos for this simplifying on the podcast references, will be a nice time saver (for me!)! ❤️

@kouloumos kouloumos deleted the podcast-references branch April 24, 2023 09:19
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.

3 participants