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

LIVE-1: embed and live main page #45

Merged
merged 16 commits into from
Apr 15, 2022
Merged

Conversation

probablybenallen
Copy link
Member

should get us all the features we need for roses with some time to test it beforehand...?

@probablybenallen probablybenallen requested a review from rmil April 13, 2022 20:14
Copy link
Member

@rmil rmil left a comment

Choose a reason for hiding this comment

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

Looks like a solid bit of work, just got a couple of questions on it. Also, no worries if not, but if you could post a screenshot of what this would look like, that would be great

<small>
YSTV will be going live soon, tune it at{" "}
{new Date(channel.scheduledStart).toLocaleTimeString(
"en-US",
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be en-GB?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know it does not affect timezones - just formatting, it's just the least faff way of converting time to 12hr with an AM and no seconds without introducing a third party library (I've been trying to keep js bloat to a minimum on public-site), en-GB does 24hr clock which didn't look as user friendly to me

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, I did see en-GB used elsewhere, are we wanting it swapped there as well?

Copy link
Member Author

@probablybenallen probablybenallen Apr 15, 2022

Choose a reason for hiding this comment

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

Nope, because that's where I wanted the date only so I need "en-GB" so it gets the day and month the right way around. Should probably add that as comments

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, yeah a comment would be great

components/HomeLiveBanner/index.tsx Show resolved Hide resolved
pages/index.tsx Outdated Show resolved Hide resolved
pages/embed/live/[embed_id].tsx Show resolved Hide resolved
components/HomeLiveBanner/index.tsx Outdated Show resolved Hide resolved
components/HomeLiveBanner/index.tsx Outdated Show resolved Hide resolved
@probablybenallen
Copy link
Member Author

For reference here is a screenshot of the design (could do with further refinement but should work as a first draft)

image

@probablybenallen probablybenallen requested a review from rmil April 14, 2022 13:42
@markspolakovs markspolakovs changed the title embed and live main page INF-1: embed and live main page Apr 14, 2022
@markspolakovs markspolakovs changed the title INF-1: embed and live main page LIVE-1: embed and live main page Apr 14, 2022
Copy link
Member

@rmil rmil left a comment

Choose a reason for hiding this comment

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

Cheers, have left a comment on the en-GB thing, but I'm content for this to be merged regardless

@probablybenallen probablybenallen merged commit 97954ec into master Apr 15, 2022
@probablybenallen probablybenallen deleted the feature_main-page branch April 15, 2022 14:38
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.

2 participants