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

Schedules on the Calendar page #501

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

MarkSort
Copy link

@MarkSort MarkSort commented Feb 4, 2023

Adds cards for live shows to the Calendar page. The day and time displayed on the cards updates based on the timezone chosen in the drop down.

This leans on the pre-existing text of the options in the timezone select. That means the text of those options will have to continue to start with (GMT followed by the offset. Otherwise this code would have to also be updated.

The code would be simpler if we pulled in some datetime library, but that also seems overkill to me when this is the only place it would be used.

The options with DST will be wrong by an hour when DST starts/ends until the options in the select are updated. That bug already exists, but it means the times in this feature will also be off by an hour when the options haven't been updated. We could maybe generate those in Hugo instead of manually, but that's probably out of scope here, just calling it out.

@MarkSort
Copy link
Author

MarkSort commented Feb 4, 2023

How it looks currently:

jupiter-calendar-schedule-2

@gerbrent gerbrent added enhancement New feature, enhancement, or request design design and visual in progress currently being worked on labels Feb 5, 2023
<iframe frameborder="0" scrolling="no" data-src="//www.google.com/calendar/embed?showTitle=0&amp;showNav=0&amp;showPrint=0&amp;showTabs=0&amp;showCalendars=0&amp;mode=AGENDA&amp;wkst=1&amp;bgcolor=%23FFFFFF&amp;src={{.Site.Params.calendar.embedd}}&amp;color=%235229A3" width="100%" height="400px"></iframe>

<select name="ctz" id="ctz" style="width: 100%;">
<h2>Regular Live Show Times</h2>
Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about how these shows have two schedules: a live show schedule and a recorded show release schedule.

Not sure how much this header helps differentiate but wanted to try something.

@MarkSort MarkSort marked this pull request as ready for review February 5, 2023 20:03
@gerbrent
Copy link
Collaborator

gerbrent commented Feb 6, 2023

Looks impressive on initial review - will give it some testing and look to some more talented folk to give comment on the implementation.

Thanks for this!

@gerbrent gerbrent added the low priority ... but not necessarily unimportant label Feb 6, 2023
Copy link
Collaborator

@CGBassPlayer CGBassPlayer left a comment

Choose a reason for hiding this comment

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

Overall a great (and I feel much needed) addition to the site! Just a few suggestions and tweaks to make this contribution that much better! Just let me know when you are ready for another review

[live]
type = "weekly"
day = 1
hour = 9
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Coder Radio card seems to be ahead of the Google Calendar by about 3 hours.

image

Copy link
Author

Choose a reason for hiding this comment

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

I think this is because today's time is not the regular time. It should match if you look at the time for next week. I can ask around too though.

But I guess it does bring up a non-technical question, is there a way we can make the reason these don't match more clear? Not something we can do in the website, but could the calendar items add "(Special Time)" to the title, or something similar?

(And I will take a swing at the other comments later, thanks!)

Copy link
Author

Choose a reason for hiding this comment

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

Chris confirmed in matrix that today's time is not the usual time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like I should've looked farther ahead! I think we the calendar should have some kind of note if possible to specify that the time is different from normal. I haven't used Google Calendar so I am not sure if that is an option. We can ask Chris about that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another crazy thought... Could we use the Google Calendar API to get the time of the next event? Maybe outside the scope of this PR (unless you really want to tackle it). But that could be a good next step on this.

Copy link
Collaborator

@CGBassPlayer CGBassPlayer Feb 8, 2023

Choose a reason for hiding this comment

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

the Hugo build, instead of client side JS, so we don't have to worry about API keys being public?

We were looking into how to handle the contact form with its API as well. I had proposed a separate backend just handle API requests like those. There was some discussion about it in #232

Copy link
Contributor

@reclaimingmytime reclaimingmytime Feb 11, 2023

Choose a reason for hiding this comment

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

so we don't have to worry about API keys being public?

We can look into settings to protect and scope the Google Calendar API key properly. Such as only allowing access to the JB Calendar from jupiterbroadcasting.com, and nothing else from Google Calendar. I haven't worked with the Google Calendar API, but I've seen other APIs allow these kinds of settings, so it should work with Google Calendar as well. Quickly glancing at the API, it seems to be possible with https://cloud.google.com/docs/authentication/api-keys#api_key_restrictions.

I see https://irc.twit.tv/ is using a solution based on the Google Calendar API, we might be able to build something similar. You can search for "calendar" in the Network tab of your browser and see what Google API they are using. I'm hoping they've done their research on not expoing their API key.

If we have an API key, this might just be a separate AJAX request to a JSON API, similar to what TWiT is using.

Edit: This seems to be the endpoint we want. https://developers.google.com/calendar/api/v3/reference/calendars/get?apix_params=%7B%22calendarId%22%3A%22primary%22%7D#http-request
What is weird is the examples from Google only list backend languages (Java, Python, PHP, and Ruby), so maybe the API is not intended to be client-side. But as CGBassPlayer mentioned in #232, we could use a separate backend service for that.

I wouldn't want to drag this out too much if the current version seems useful as a first iteration.

I agree, but I think if we change the calendar, we should make sure it's correct and not too confusing. I think the design of the cards looks great, and it's definitely a great addition. Just hard-coding the events might make it more confusing. A better solution could be using the Google Calendar API, in my opinion.

Copy link
Author

Choose a reason for hiding this comment

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

I did get excited to have something merged here, but makes sense to back up and reassess. 😅

Are we still talking about adding "Next Showtime" to the cards? And potentially using the Google Calendar API to determine that?

We'd still need to indicate when the "Next Showtime" is not a regular show time. We could maybe do that by calculating when the next regular time should be, and if they don't match, we know it's special.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could maybe do that by calculating when the next regular time should be, and if they don't match, we know it's special.

That could work. Maybe looking at the last 3-4 shows and determining a pattern could be an idea.

We should also take permanent changes in the live calendar into account, like how LUP was moved from Tuesday to Sunday.

I also noticed that some event in the calendar have a different name to make it clear it was a different live time.

image

Maybe this will make it more difficult to automatically detect which show is live? We could detect any show with "Coder Radio" in the title, but what about false positives like "No Coder Radio this week"? Have we had this before? 😁

Or maybe it's good enough to have a system that works most of the time? Google has changed the API before, and there will be changes in the future, so there might always be unforeseen breakages.

Copy link
Author

Choose a reason for hiding this comment

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

"No Coder Radio this week"

Nevermind I give up.

Just kidding, but really my goal here was to have somewhere on the website to find general live schedules for each show. We immediately hit a corner case where it was confusing, so it became more difficult to proceed. If I have time I'll check out the Google Calendar API.

Maybe looking at the last 3-4 shows and determining a pattern could be an idea.

I'd hope we can be more precise, either by looking at the recurrence of the Google Calendar events, or having something like I have in the live property of shows in this PR.

themes/jb/layouts/calendar/single.html Outdated Show resolved Hide resolved
themes/jb/layouts/partials/episode/schedule.html Outdated Show resolved Hide resolved
@MarkSort MarkSort requested review from CGBassPlayer and removed request for elreydetoda February 8, 2023 02:03
@CGBassPlayer
Copy link
Collaborator

Initial scroll through the PR looks good. I will give a real review tomorrow

Copy link
Collaborator

@CGBassPlayer CGBassPlayer left a comment

Choose a reason for hiding this comment

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

I think this PR is good as is for now. But see my comment about a future PR.

In local testing:

  • dev container built and ran
  • basic click through testing looked good
  • e2e tests passed

Comment on lines 330 to 350
<br>
<br>

// Detect timezone automatically
var tz = Intl.DateTimeFormat().resolvedOptions().timeZone;
var tzname = encodeURIComponent(tz);
document.querySelector('iframe').src = calendarBase + "&ctz=" + tzname;
<div class="container">
<div class="columns is-multiline">
{{ range where .Site.Pages "Section" "show" }}
{{ if (isset .Params "live") }}
<div class="column is-6 is-3-fullhd is-3-desktop is-12-mobile" style="display: flex;">
{{ partial "episode/schedule.html" . }}
</div>
{{ end }}
{{ end }}
</div>
</div>

// Change on timezone after dropdown
document.querySelector('#ctz').onchange = function () {
var tzname = document.querySelector('#ctz').value;

if(tzname && tzname != "none") {
document.querySelector('iframe').src = calendarBase + "&ctz=" + tzname;
}
};
</script>
<br>
<br>

Subscribe To The Calendar: <a href="{{.Site.Params.calendar.rss}}">iCalendar</a>

<iframe frameborder="0" scrolling="no" data-src="//www.google.com/calendar/embed?showTitle=0&amp;showNav=0&amp;showPrint=0&amp;showTabs=0&amp;showCalendars=0&amp;mode=AGENDA&amp;wkst=1&amp;bgcolor=%23FFFFFF&amp;src={{.Site.Params.calendar.embedd}}&amp;color=%235229A3" width="100%" height="400px"></iframe>
Copy link
Collaborator

Choose a reason for hiding this comment

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

So personally I try to avoid <br/> for spacing content and untagged text in my html. I would use the classes provided by Bulma to handle that with CSS.

I am thinking this is fine for this PR. But a follow up (of either a calendar rework using or of a styling change) could be where this will happen.

Copy link
Author

Choose a reason for hiding this comment

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

I added a div and some Bulma classes and removed the <br>s.

@CGBassPlayer CGBassPlayer removed the in progress currently being worked on label Feb 8, 2023
@elreydetoda elreydetoda mentioned this pull request Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design design and visual enhancement New feature, enhancement, or request low priority ... but not necessarily unimportant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants