Skip to content

"JB is live!" indicator - on page load, call the script to highlight the #livebutton red if jupiter.tube is live. #378

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

Merged
merged 9 commits into from
Sep 11, 2022

Conversation

DECbot
Copy link
Contributor

@DECbot DECbot commented Sep 2, 2022

Adding 'id' attribute to the live button in nav bar for dynamically setting background color when JB is live.

Adding 'id' attribute to the live button in nav bar for dynamically setting background color when JB is live.
@elreydetoda
Copy link
Collaborator

elreydetoda commented Sep 2, 2022

Going to close your other PRs (#379 & #380) based on the feedback (in matrix) of consolidating them all into this one:

You can do it off a single pull request. If you're only making submissions via the web-ui, you make the same modifications on this specific branch: https://github.com/DECbot/jupiterbroadcasting.com/tree/patch-1

It'll submit all your changes via the one pull request: #378

Then you can close the other ones 🙂

It won't delete them, but just let others know not to merge them.

Also, do you mind putting this PR into draft mode till you do consolidate them all?

This was referenced Sep 2, 2022
@DECbot DECbot marked this pull request as draft September 2, 2022 19:29
@elreydetoda
Copy link
Collaborator

Thank you! Also, thank you for wanting to do this feature @DECbot!

I can't wait for this feature to be added! This'll be so cool 🥳 🎉 🚀

checks the jupiter.tube api for live shows, if count is greater than 0, the #livebutton background-color style is updated to red.
@DECbot
Copy link
Contributor Author

DECbot commented Sep 2, 2022

I believe this is what you had in mind. Thanks for coaching me through this.

@DECbot DECbot marked this pull request as ready for review September 2, 2022 19:42
@gerbrent gerbrent changed the title Update header.html "JB is live!" indicator - on page load, call the script to highlight the #livebutton red if jupiter.tube is live. Sep 2, 2022
@elreydetoda
Copy link
Collaborator

I believe this is what you had in mind.

That's perfect!

Thanks for coaching me through this.

Anytime! Just glad to have your contribution 😁 🥳

I still haven't reviewed it yet, but it's looking like it's ready for review now if someone else has some time 🙃

@ironicbadger
Copy link
Collaborator

Screenshot please!

@DECbot
Copy link
Contributor Author

DECbot commented Sep 3, 2022

Logic inverted in console for demonstration since jupiter.tube is not live. Formatted in the console as a single liner to appease Firefox.

image

@gerbrent
Copy link
Collaborator

gerbrent commented Sep 3, 2022

That is pretty sexy. Nicely done!

@gerbrent
Copy link
Collaborator

gerbrent commented Sep 6, 2022

"JoopTubeQuery" ; )

Copy link
Collaborator

@gerbrent gerbrent left a comment

Choose a reason for hiding this comment

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

Seems ok from my surface-level understanding. Looking for other reviewers to confirm..

@gerbrent
Copy link
Collaborator

gerbrent commented Sep 7, 2022

Would love to get this merged (if acceptable) before Sunday....

@gerbrent
Copy link
Collaborator

gerbrent commented Sep 7, 2022

We could also simulate a live show from JB One if needed for testing.... just let me know!

@gerbrent gerbrent added the urgent label Sep 7, 2022
@DECbot DECbot requested review from elreydetoda and removed request for StefanS-O September 11, 2022 05:49
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.

Overall I think it's an awesome MVP of what we can do! 🥳

I think I've just got some small feedback on not needed hugo HTML/logic, and also just trying to make the code more DRY.

Once those changes get merged this should be good to go 😁 , thanks again @DECbot and sorry it took so long to review 😅 . Work got pretty crazy this week 🙃 , but thank you for your contribution and I'm super excited about this PR 🥳 🚀 🎉

P.S. I've got extra comment/info + possible future improvements, so do not worry about doing the stuff below (just my inline GH comments)


BTW, here's what it'd look like in the mobile dropdown

image


Event that I used to test (taken from a while back), and I used Burp Suite Community Edition to intercept and modify the response and test what a "valid" response would be.

{"total":1,"data":[{"id":50,"uuid":"55b2e9d1-ccf3-4914-9f3f-c5d8a59bd4c1","shortUUID":"bzMpxgG2rDKn9C99GSab92","url":"https://jupiter.tube/videos/watch/55b2e9d1-ccf3-4914-9f3f-c5d8a59bd4c1","name":"LINUX Unplugged 471 - We Broke Our Server","category":{"id":15,"label":"Science & Technology"},"licence":{"id":5,"label":"Attribution - Non Commercial - Share Alike"},"language":{"id":"en","label":"English"},"privacy":{"id":1,"label":"Public"},"nsfw":false,"description":"We broke our server, so lets see if we can fix it live before the show is over!","isLocal":true,"duration":0,"views":0,"viewers":19,"likes":0,"dislikes":0,"thumbnailPath":"/static/thumbnails/b610e5f5-bbb5-4ee1-ac24-421c50f0daf5.jpg","previewPath":"/lazy-static/previews/fcd582d8-68c5-4f00-8a4a-0a8ffde1a4a5.jpg","embedPath":"/videos/embed/55b2e9d1-ccf3-4914-9f3f-c5d8a59bd4c1","createdAt":"2022-08-14T16:26:27.938Z","updatedAt":"2022-08-14T18:53:15.544Z","publishedAt":"2022-08-14T18:53:15.543Z","originallyPublishedAt":null,"isLive":true,"account":{"id":3,"displayName":"JBLive Stream","name":"jblive","url":"https://jupiter.tube/accounts/jblive","host":"jupiter.tube","avatars":[{"width":48,"path":"/lazy-static/avatars/e4d17e7f-a144-4a4a-b5d8-b297b06b727f.png","createdAt":"2022-06-07T23:43:56.565Z","updatedAt":"2022-06-07T23:43:56.565Z"},{"width":120,"path":"/lazy-static/avatars/1340f3e9-d0a7-4bc4-bcbd-a8c865eaf1b8.png","createdAt":"2022-05-30T20:36:19.005Z","updatedAt":"2022-05-30T20:36:19.005Z"}],"avatar":{"width":48,"path":"/lazy-static/avatars/e4d17e7f-a144-4a4a-b5d8-b297b06b727f.png","createdAt":"2022-06-07T23:43:56.565Z","updatedAt":"2022-06-07T23:43:56.565Z"}},"channel":{"id":2,"name":"live","displayName":"live","url":"https://jupiter.tube/video-channels/live","host":"jupiter.tube","avatars":[{"width":48,"path":"/lazy-static/avatars/8acfd2a2-ab4e-48aa-990c-3156a2765d2e.png","createdAt":"2022-06-07T23:43:56.607Z","updatedAt":"2022-06-07T23:43:56.607Z"},{"width":120,"path":"/lazy-static/avatars/e9bcfdb7-90a2-479c-b1da-1ab0e5fc9442.png","createdAt":"2022-05-30T20:38:33.632Z","updatedAt":"2022-05-30T20:38:33.632Z"}],"avatar":{"width":48,"path":"/lazy-static/avatars/8acfd2a2-ab4e-48aa-990c-3156a2765d2e.png","createdAt":"2022-06-07T23:43:56.607Z","updatedAt":"2022-06-07T23:43:56.607Z"}}}]}

here is the spot that makes it "valid", because the length of the events is greater than 0.

image


possible future improvements:

  • don't make red on live page. (IMO distracts from live show video)
  • someway to display (outside of menu (in case people don't click on menu)) to mention that we're live (maybe pop-up once and gently notify them?)
  • agree with @ChanceM (source), and this should probably be a class not just an inline style

@elreydetoda
Copy link
Collaborator

elreydetoda commented Sep 11, 2022

also, @DECbot I saw you were going to push some tests in for this as well (source).

I'd honestly make that another PR, and we can work through refining tests for this 😁 . Typically I'd prefer to get test in with the code, but there are some pretty cool feature with playwright were you can intercept and modify the response to "mock" a true answer to the jb endpoint response. Which I think would be really useful in this instance to ensure we can validate the background changing to red.

@DECbot
Copy link
Contributor Author

DECbot commented Sep 11, 2022

also, @DECbot I saw you were going to push some tests in for this as well (source).

I'd honestly make that another PR, and we can work through refining tests for this grin . Typically I'd prefer to get test in with the code, but there are some pretty cool feature with playwright were you can intercept and modify the response to "mock" a true answer to the jb endpoint response. Which I think would be really useful in this instance to ensure we can validate the background changing to red.

Yes, that is my intention, to do a follow-up pull request after this one is accepted to include e2e testing. I didn't know Playwright could intercept API calls--I was intending to inject the JSON response. Intercepting and modifying would be a much better way to do it.

I liked the suggestion to do this with classes rather than inline styles. As for a notifier on the mobile layout, I want to figure out how to paint a red dot on the bottom corner of the uncollapsed menu when live much like how Android draws a dot on apps with notifications.

For not painting the live button on the live page, I assume Hugo can selectively exclude the doLiveHighlight function call in the live/jb-tube.html partial. That's the laziest solution I can think of at the moment.

@DECbot DECbot requested a review from elreydetoda September 11, 2022 15:10
Copy link
Contributor Author

@DECbot DECbot left a comment

Choose a reason for hiding this comment

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

changes made

@elreydetoda
Copy link
Collaborator

Yes, that is my intention, to do a follow-up pull request after this one is accepted to include e2e testing. I didn't know Playwright could intercept API calls--I was intending to inject the JSON response. Intercepting and modifying would be a much better way to do it.

Ya, playwright has a lot of cool features that I'm honestly still learning about 😅 . I've only been using it for about a month or so, but have really been trying to dive in and leverage all it's functionality 😁. For example I just recently learned about that playwright can emulate all kinds of devices based on screen width/height + user-agent 🙂

As for a notifier on the mobile layout, I want to figure out how to paint a red dot on the bottom corner of the uncollapsed menu when live much like how Android draws a dot on apps with notifications.

Ah, now that sounds really cool! 😁 Ya, doing that in a future PR would be pretty awesome 🥳

For not painting the live button on the live page, I assume Hugo can selectively exclude the doLiveHighlight function call in the live/jb-tube.html partial. That's the laziest solution I can think of at the moment.

tl;dr: yes it does 😁

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.

Everything else looks good to me 😁

@DECbot DECbot requested a review from elreydetoda September 11, 2022 16:19
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.

Looks good to me 🥳🎉

@elreydetoda elreydetoda merged commit d989238 into JupiterBroadcasting:main Sep 11, 2022
@reesericci
Copy link
Collaborator

I personally think that it should be more obvious that JB is live, rather than a small red button in the navbar. Something like #34 - should I work on a PR for that?

@gerbrent
Copy link
Collaborator

hmm!
Well..... it'll be a matter of design opinion on this one, likely from JB (should get internal votes).

I'd be curious to hear what others here think aswell. Has the current LIVE indicator been sufficiently obvious?

#378
image

Happy to reopen this to discussion!

@DECbot
Copy link
Contributor Author

DECbot commented Oct 11, 2022

I'm all for the script toggling a css class instead of doing an inline css style change. With a class multiple things can change when live--like not hiding a div with the live episode details.

@DECbot
Copy link
Contributor Author

DECbot commented Oct 11, 2022

I admit, I'm bias because I know why that box is red, but I've caught more live shows because of the box than on the old website.

@gerbrent gerbrent mentioned this pull request Oct 28, 2022
20 tasks
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.

5 participants