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

Add episode numbers/prefix in RSS and Relevant Titles/Links [Attempt #2] #322

Merged
merged 2 commits into from
Dec 22, 2020
Merged

Add episode numbers/prefix in RSS and Relevant Titles/Links [Attempt #2] #322

merged 2 commits into from
Dec 22, 2020

Conversation

chrisreddington
Copy link
Contributor

Signed-off-by: Chris Reddington [email protected]

This is an PR to address issues #168 and #67. This is the second attempt (after this PR #313), due to accidentally wiping away the changes. This PR has been made by taking the diffs from the old tree and applying them to the latest version of upstream master.

Fixes #168
Fixes #67

The intent is to ensure consistency across the site in using either the parentheses, brackets or dash format to display the episode number (with an optional prefix).

Changes have also been made to the title tag, as per one of the issues outlined above

I believe this would also close #247
Fixes #247

@mattstratton - This also includes the suggestion at making reference.MD and config.toml consistent - #313 (comment)

Signed-off-by: Chris Reddington <[email protected]>
<div class="col">
<h1>{{ title .Title }}</h1>
<div class="col">
{{ if isset .Site.Params "episode_number_style"}}
Copy link
Owner

Choose a reason for hiding this comment

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

Might want to check for both if episode_number_style is set as well as .Params.episode since if the episode doesn't have one set, it will error (it's possible that older episodes might not have them set, etc etc?)

Copy link
Contributor Author

@chrisreddington chrisreddington Dec 10, 2020

Choose a reason for hiding this comment

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

Ah, good catch. So refactoring all instances of the conditional to be something like {{ if and (isset .Site.Params "episode_number_style") (isset .Params "episode") }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making a slight alteration - Will use if and (isset .Site.Params "episode_number_style") (.Params.episode) }}. That then accounts for the empty string scenario. Will also add an else to that initial if, to cover the title on its own, and then we're wrapped up.

@@ -108,7 +108,17 @@ <h3>Episodes Featuring {{ .Title }}</h3>
{{- range $page := (where ( where site.RegularPages "Type" "in" site.Params.mainSections) ".Params.upcoming" "!=" true ) -}}
{{- range $page.Params.guests -}}
{{- if eq . ($.Scratch.Get "guest-name") -}}
<a href = "{{$page.Permalink}}" class = "guest_page_episode_link list-group-item list-group-item-action">{{$page.Title}}</a>
{{ if isset $.Site.Params "episode_number_style"}}
Copy link
Owner

Choose a reason for hiding this comment

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

add the check for .Params.episode here as well (basically the same if AND statement)

@@ -87,7 +87,17 @@ <h3>Episodes Hosted By {{ .Title }}</h3>
{{ range $page := (where ( where site.RegularPages "Type" "in" site.Params.mainSections) ".Params.upcoming" "!=" true ) }}
{{ range $page.Params.hosts }}
{{ if eq . ($.Scratch.Get "host-name") }}
<a href = "{{$page.Permalink}}" class = "guest_page_episode_link list-group-item list-group-item-action">{{$page.Title}}</a>
{{ if isset $.Site.Params "episode_number_style"}}
Copy link
Owner

Choose a reason for hiding this comment

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

you know where I am going with this...

@mattstratton
Copy link
Owner

I would just suggest adding the check for .Params.episode in the first if (I mentioned in a few places where it occurs, but you will want to extend it to all the places). Otherwise, this seems good!

@chrisreddington
Copy link
Contributor Author

@mattstratton - Tweaked the first if, and also made sure there is an else, which links the "Standard" title without any episode number/abbreviation.

@chrisreddington
Copy link
Contributor Author

@mattstratton - Any additional concerns on this one? Otherwise, it looks like we can get 3 of the current open issues closed out :)

@mattstratton
Copy link
Owner

I think this looks good. I'm holding off on merging a lot of these since I want to bundle them into a release and keep track of what happens in that release (granted, my release script will pick up the issues/PRs, but since I know it will be a few days before I can cut a release, I'm just holding off on these merges)

@mattstratton mattstratton merged commit 3db321a into mattstratton:master Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feed improvements Allow for customization of titles in feed Add episode numbers as an option
2 participants