-
Notifications
You must be signed in to change notification settings - Fork 137
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
Conversation
b5ce462
to
fd58b3f
Compare
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 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. |
fd58b3f
to
c1c765e
Compare
PR updated with @harding's commit separation request.
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).
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.
There was a problem hiding this 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.
34b50d9
to
c73b42c
Compare
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. |
c73b42c
to
9724a7a
Compare
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! |
Thank you again to @kouloumos for this simplifying on the podcast references, will be a nice time saver (for me!)! ❤️ |
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:
New logic:
reference
variable in the frontmatterFor each podcast, the
recap_references_generator
will parse thereference
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.