Skip to content

Conversation

@OscarFava
Copy link
Contributor

What is this PR doing?

This PR is adding the feature: background replacement.

How should this be manually tested?

What are the relevant tickets?

A maintainer will add this ticket number.

Resolves VIDSOL-105

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?

@OscarFava OscarFava added the do-not-merge Do Not Merge label Jul 21, 2025
@OscarFava OscarFava changed the base branch from main to develop July 21, 2025 11:15
@odivorra
Copy link

odivorra commented Jul 23, 2025

@OscarFava , quick question:
under which license are we able to include the pictures you are adding here ?
I see some pictures that could be subject to copyright, unless you know otherwise. Checking to make sure we don't add any content we do not have the right to add.

@OscarFava
Copy link
Contributor Author

OscarFava commented Jul 23, 2025

Hi @odivorra I have used the free library mentioned in the Jira ticket (https://jira.vonage.com/browse/VIDSOL-105):
image

…with unified management in preview and meeting rooms. Refactored publisher logic, updated user context, and enhanced UI to support and display background selections.
…dded isParentVideoEnabled prop, standardized option sizing, and improved disabled video container styles.
@OscarFava OscarFava removed the do-not-merge Do Not Merge label Jul 31, 2025
@OscarFava OscarFava requested review from behei-vonage and cpettet and removed request for cpettet July 31, 2025 13:47
Copy link
Contributor

@cpettet cpettet 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 so far! Great work with this! Just have a few comments/questions. Let me know what you think!

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.

great job with this feature, Oscar! left a few questions/comments. thanks! 🙏

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.

thanks for making the changes so far 🙏 just left a few more things. thanks!

Copy link
Contributor

@cpettet cpettet 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 so far! Just have a few comments/questions. Let me know what you think!

Copy link
Contributor

@cpettet cpettet 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 so far! Just have a few comments/questions. Let me know what you think!

@OscarFava OscarFava requested a review from cpettet August 7, 2025 07:13
Copy link
Contributor

@cpettet cpettet 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 so far! Just have one more. Let me know what you think!

@sonarqubecloud
Copy link

Copy link
Contributor

@cpettet cpettet 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
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! 🚀

@dwivedisachin
Copy link
Contributor

LGTM!!
A few small observations but those can be addressed on a follow up PR.

  • On small Mobile devices the scroll comes up on the whole page, I believe it should be on only the choose background effects section.
  • The Cancel and Apply buttons should be at the bottom and always visible.
  • On waiting room mobile view the popup does not take up the whole screen (Not sure if this is necessary though).
  • Video list the one with "Background Effects" should be auto closed whe user clicks on "Background Effects" (It looks really weird on mobile view).

Let me know what do you think. 🤔

I'll merge this PR.

@dwivedisachin dwivedisachin merged commit bb15256 into develop Aug 19, 2025
7 checks passed
@dwivedisachin dwivedisachin deleted the vidsol-105/background-replacement branch August 19, 2025 09:56
@OscarFava
Copy link
Contributor Author

LGTM!! A few small observations but those can be addressed on a follow up PR.

  • On small Mobile devices the scroll comes up on the whole page, I believe it should be on only the choose background effects section.
  • The Cancel and Apply buttons should be at the bottom and always visible.
  • On waiting room mobile view the popup does not take up the whole screen (Not sure if this is necessary though).
  • Video list the one with "Background Effects" should be auto closed whe user clicks on "Background Effects" (It looks really weird on mobile view).

Let me know what do you think. 🤔

I'll merge this PR.

First, second and fourth suggestion will be done.
Third suggestion remains the same (Dialog must have margins).

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.

6 participants