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

VIDCS-3040: There is a difference in publisher preview on waiting room and the meeting room #65

Merged
merged 12 commits into from
Feb 19, 2025

Conversation

cpettet
Copy link
Contributor

@cpettet cpettet commented Feb 12, 2025

What is this PR doing?

Description

For mobile devices, the preview publisher in the waiting room and the one displayed for subscribers in the meeting room don't match up. See screenshots below, notice the presence or absence of the shoe tower in the before shots.

Screenshots (after)

Screenshot 2025-02-12 at 10 46 18 AM
Screenshot 2025-02-12 at 10 46 28 AM

Screenshots (before)

Screenshot 2025-02-12 at 10 48 21 AM
Screenshot 2025-02-12 at 10 48 39 AM

How should this be manually tested?

Repro'ing the issue
  • checkout develop or go to the live link
  • open a browser in a mobile view
  • go to the waiting room
  • note the preview publisher display
  • enter waiting room
  • note the publisher display shows a lot more than was displayed
Verifying the fix
  • checkout this branch
  • open a browser in a mobile view
  • go to the waiting room
  • note the preview publisher display
  • enter waiting room
  • note the publisher display and preview publisher displayed are more or less the same
Trying on real devices
  • double-check the fix with any mobile devices you have (please ping me if needed for ngrok)

What are the relevant tickets?

A maintainer will add this ticket number.

Resolves VIDCS-3040

Checklist

[✅] Branch is based on develop (not main).
[✅] Resolves a Known Issue.
[✅] If yes, did you remove the item from the docs/KNOWN_ISSUES.md?
[ ] Resolves an item reported in Issues.
If yes, which issue? Issue Number?

@cpettet cpettet added the update-screenshots Run update screenshots CI workflow label Feb 12, 2025
@cpettet cpettet closed this Feb 12, 2025
@cpettet cpettet reopened this Feb 12, 2025
cpettet and others added 6 commits February 18, 2025 08:56
…c.ts-snapshots/Waiting-page-UI-test-1-Electron-linux.png
…c.ts-snapshots/Waiting-page-UI-test-1-Google-Chrome-Fake-Devices-linux.png
…c.ts-snapshots/Waiting-page-UI-test-1-Microsoft-Edge-linux.png
…c.ts-snapshots/Waiting-page-UI-test-1-Opera-linux.png
…c.ts-snapshots/Waiting-page-UI-test-1-firefox-linux.png
Copy link
Contributor

@behei-vonage behei-vonage left a comment

Choose a reason for hiding this comment

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

LGTM! great job :shipit:

Copy link
Collaborator

@maikthomas maikthomas left a comment

Choose a reason for hiding this comment

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

LGTM but this needs testing on real devices to make sure it works as we expect 🙏

console.error('initPublisher error: ', err);
publisherRef.current = initPublisher(
undefined,
{ insertDefaultUI: false, resolution: '1280x720' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we double check this is doing what we expect on real devices too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the test instructions to test on real devices. FWIW, it looked okay on my iOS device

@DeliaTok
Copy link
Collaborator

looks good!

@cpettet cpettet merged commit 5dea6e7 into develop Feb 19, 2025
9 checks passed
@cpettet cpettet deleted the cpettet/vidcs-2983-waiting-room-cut-off branch February 19, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
update-screenshots Run update screenshots CI workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants