-
Notifications
You must be signed in to change notification settings - Fork 51
Feature/automate sponsors page #451
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
Feature/automate sponsors page #451
Conversation
…upiterbroadcasting.com into feature/automate-sponsors-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.
This looks great! Worked well in local testing as well
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.
So...first off I want to say if we do decide to proceed with this, the logic for how @ChanceM implemented it is solid! I honestly think that the way he did it was the best way anyone could have 🥳, so very very nicely done @ChanceM! 🥳 🎉
Besides some minor aesthetic (only a few of them am I requesting changes for) stuff and some questions for clarification, there are some quirky effects from automating the sponsors updates directly from the JB episodes. This is by no way anything that @ChanceM could affect with this PR, but is a larger conversation that probably needs to be had about how JB publishes ads on each episode and the scraper.
Downside to automation
So, one of the biggest downsides of using the automation is not being able to make small tweaks in verbiage like how @gerbrent just did with #447.
I personally don't think that's an issue as long as
- the JB ads (that are published) are worded in ways that work with the website in mind
- the scraper always updates the ad with the latest version.
Situation 1 issues
current issues (left side is this PR & right side (if applicable) is main website :
- For right now the podcast episodes for LUP (example) don't point to the right place (according to 447)
- currently this PR would be a partial regression of update sponsorship info #447 since it's pointing to just tailscale.com.
- Another small thing would be you can't add comments with MD
- I don't think this is bad, just something of note
- might have to "blacklist certain sponsors since Office Hours uses jupiter.party as a sponsor which is not currently listed:
- also, slight side-note the title case is weird if
www
in-front (which a future sponsor might have)
- also, slight side-note the title case is weird if
- Different name in episodes than typically/currently used (Linode vs Linode Cloud Hosting (e.g. episode)):
- Duplication of sponsor name in description, which is an example of how JB's currently only thinking of what looks good on the episode pages for ads (could probably be tweaked for website):
- fixed with: cefc682
Situation 2 issues
Currently it's a known issue (but I don't think it's documented anywhere) that a sponsor's description could change between scraper runs. This is because the scraper will select the first episode it can parse which isn't already added to the SPONSOR
global variable for the corresponding show (logic is here).
If we were able to pull the most recent episode it would negate the largest overall issue, because then JB could update their ad and it would reflect on the website (technically only for the most 5 recent episodes though with our current setup).
I've added this issue/situation to my tasks listed in JupiterBroadcasting/show-scraper#20 as an OOP enhancement, because we'd be able to prevent this if we could say "only use the most recent sponsor's description" instead of leaving it to chance. This doesn't technically currently affect us at all because the sponsor content isn't overwritten if it already exists (since we're doing the LASTEST_ONLY
environment variable for the GH action in this repo).
small aesthetic stuff
inconsistent {{-
or -}}
, intentional?
delete all the content from _index.md
Lose a bit of spacing that I personally liked to structure the hierarchy of the page (not a designer though 🙃)
Other info:
Great stuff! I was remarking to Chris just how great this is as we were driving down the road on the last leg of our JPL Road trip this week... thanks @ChanceM ! |
There is a "Jupiter Broadcasting" section that is omitted from the PR version that would be great to integrate.
This section is quite helpful for network-wide sponsorship links, and doesn't really appear in episodes regularly. Having a way of adding non-automated links occasionally would be helpful aswell, for instance as described in #458 |
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.
Awesome! Everything looks really good still 🙃 . One small thing to help us find and update this section once the JB ads are a bit more mindful of the website's automated collection.
Besides that and the addition to handle JB network wide sponsors through the _index.md
, I think it'll be ready (from a code perspective, but @gerbrent might still have feedback for the aesthetics) for a merge 🥳
BTW, I didn't pull it down (this time) I just looked at it in GH from a purely code perspective.
nice changes @ChanceM ! Will get to testing this later this week... seems just about ready from what I can tell. |
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.
I think overall this looks good to me 😁 🥳
Made a small comment, which also outlines the process (which you'd already done once) for adding network wide sponsorships.
There are still 3 things outstanding
- @gerbrent's comment about the line breaks (don't know where we've fallen on if it'll be removed or not).
- the manually adding sponsors mentioned here
- I imagine they could follow a similar process I outlined for network wide memberships. instead of using the
_index.md
for the sponsors page, they could use the_index.md
for the corresponding show page. - Since you're already using those here you could probably just append those to the array if they exist with a hugo
with
- I imagine they could follow a similar process I outlined for network wide memberships. instead of using the
- resolve merge conflict
@gerbrent Do you want me to remove the line length wrap? (limited to 75 ch in the current pr) reading line length is recommended to be 45-80 ch? FOR JB: Should there be a blocked sponsor list to remove Jupiter.Party as it is currently listed under Office Hours? |
thanks @ChanceM ! Great questions! Line length: I think we should revisit the line length more globally, so keeping line length limiting OFF for now is ok, knowing we can then (as a separate PR/Issue) apply that line limiting principle to all pages that require it in a cohesive way. (I'll let @elreydetoda elaborate w technical) Blocked sponsor: |
…upiterbroadcasting.com into feature/automate-sponsors-page
@ChanceM - you're super fast! ; ) |
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.
Only small suggestion since we'd added the site wide sponsors section.
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.
Awesome! I think I've already said it multiple times, but nicely done on implementing this feature @ChanceM it was a really solid implementation to start with and still looks awesome!
(small comment on how we can add "permanent" sponsors per show)
Issues created from this PR:
Resolves #54 - Adds a param to config.toml for number of days to look back.