Skip to content

Conversation

@duckunix
Copy link

This is for #136 and is a Hacktoberfest entry.

Todo:

  • Update UI
  • Update episode code to add needed header material

Items for discussion:

  • Is the location a good one?

@CGBassPlayer
Copy link
Collaborator

First off, thank you for contributing!

While you are working on this, would you mind converting this PR to a draft. There should be some text between Reviewers and Assignees

@duckunix duckunix marked this pull request as draft October 15, 2022 15:54
@duckunix
Copy link
Author

Thanks, @CGBassPlayer . I did not see the draft setting, hence the WIP in the title. I have set it now.

@CGBassPlayer
Copy link
Collaborator

No problem! After the PR is created, the button to convert is super hidden in my opinion. But while you are creating one, there is a dropdown next to the create PR that lets you pick whether or not you would like to be a draft.

PR Selection

Copy link
Collaborator

@elreydetoda elreydetoda left a comment

Choose a reason for hiding this comment

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

So, first off (like @CGBassPlayer mentioned) thank you for submitting a PR to the repo 🥳, we really appreciate contributions 😁

Down to the review/feedback 🙃

I think if you're going for a minimum amount of effort, then you could just accept my suggestion and you should have a majority of the issue implemented.
The only thing you wouldn't get is the shownotes link...more on that in a second 🙃... (here's an example of what you'd probably need to implement for this)

The other option is a bit more work, but would help JB to have less to maintain in the future (i.e. preferred IMO). I did a bunch of testing locally to try and figure out how to best implement a more DRY approach, so we could minimize the amount of code we have to maintain. It starts to be a bit more complex since when you're approaching it from an episodes perspective you shouldn't have an episode number (for the shownotes).

I created an example commit here as to what that'd look like. Essentially you'd move the range and if statements into a shared partial, and from that you'd pass the necessary contexts into that shared partial. For the episodes one it's just an empty string, so it shouldn't try to add an episode number (since there's a with there). This would allow us to re-use those templates in both the episode and the show templates link template.

Here is the branch I was testing this code out on if you want to peruse the commits: https://github.com/elreydetoda/jupiterbroadcasting.com/tree/duckunix-feature/136-feeback-button

I think that should cover everything you're looking for in that issue from a technical solution perspective, but I'll default to the more design oriented people on that 🙃 (in the past @gerbrent/@reesericci).

<a class="link level-item pr-3" target="_blank" href="{{ (.GetPage `/membership`).Permalink }}" title="support">
<div class="title fa fa-usd fa-3x"><span class="sr-only">support</span></div>
</a>
{{with .Params.links}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, there's actually a really nice construct (.Parent) in Hugo that allows you to "traverse" up the file tree, so you can access files/parameters from parent files. So, adding that here would allow you to get access to all the links that the parent show has.

Suggested change
{{with .Params.links}}
{{with .Parent.Params.links}}

Copy link
Author

Choose a reason for hiding this comment

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

That was what I was missing. :)
With that bit of knowledge, I think I can get the separate version worked out, then on to looking at DRY, which I am all for (a good SA is a Lazy SA and reuses whenever possible :) )

Copy link
Author

Choose a reason for hiding this comment

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

And I now have the extra file working. Time to refactor down to one file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! 🥳

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