Skip to content

Add Mobile Support for Live Status Indicator #487

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 4 commits into from
Jan 3, 2023

Conversation

ChanceM
Copy link
Contributor

@ChanceM ChanceM commented Dec 6, 2022

Adds a small red circle indicator to mobile menu for live status.

image

@CGBassPlayer CGBassPlayer self-requested a review December 7, 2022 17:12
@CGBassPlayer
Copy link
Collaborator

I will look at this later today. LOVE the feature though

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.

Sorry for the delay, had to get docker working again on my system.

This LGTM, I tested locally during the LUP recording and it seemed to work just fine. Fantastic addition @ChanceM!

@CGBassPlayer CGBassPlayer self-requested a review December 11, 2022 21:12
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.

Had a cache issue after reviewing. But this does still work

@gerbrent
Copy link
Collaborator

nice work both of you!

@elreydetoda
Copy link
Collaborator

elreydetoda commented Dec 31, 2022

This looks awesome, and I love the pulse effect you added @ChanceM!

To allow me to test this (since nothing is live right now 🙃 ) I figured I'd try and get the mocking (we've talked about before) a try 🙃. That way I could see what this actually looks like instead of just understanding how it works with the code 🙂

And I've got a preliminary setup working 😁

Here is just the regular (non-mobile) experience of me loading up and event I'd linked in the smoke test issue, while I'm running the test in Playwright Debug mode (so I can stop it and debug things).

image

Then this is the addition that @ChanceM made 😁 , current I'm just setting it to mobile view in my dev tools but there's device sizes we can use to modify the viewport during a test to trigger it for our actual testing.

image

I'll try and make a PR against your PR soon (don't know how much more I'll need to do) @ChanceM, so that we can submit tests along with this new feature 🙂

@elreydetoda
Copy link
Collaborator

Alright, now have mobile testing support working. Just going to build the actual test and then submit the PR 😁

image

@elreydetoda
Copy link
Collaborator

alright, I submitted a PR against your branch. It checks for the red indicator on a regular and 3 mobile devices (not really mobile devices, but just their viewports), and saves their screenshots.

As I mentioned in the PR, it's about 95% working...the only thing not working properly is the screenshot after I expand the menu. Its getting the needed info, but it's also really wide for some reason.

Between screenshots I'm testing for the various live indicators.

Besides the tests everything else looks good to me, nice job @ChanceM 😁

@gerbrent
Copy link
Collaborator

gerbrent commented Jan 3, 2023

This is amazing @elreydetoda !!! Nice idea, and great execution. Gosh you're all so great!!

@elreydetoda
Copy link
Collaborator

actually going to merge to develop and then merging my tests on top off that so we can chat about it during Office hours. 😁

@elreydetoda elreydetoda merged commit 4e3f5c8 into JupiterBroadcasting:develop Jan 3, 2023
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.

4 participants