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

Breakout rooms banner fix for react 17 and lower #5358

Merged

Conversation

mgamis-msft
Copy link
Contributor

@mgamis-msft mgamis-msft commented Oct 24, 2024

What

Fix BreakoutRoomsBanner component to return null instead of undefined when there is no breakout room state because React versions lower that 18 will throw an error if components return undefined according to this discussion post reactwg/react-18#75

Why

Identity pass found this error on mobile form factor when joining a group call with the Call composite

How Tested

Tested local sample and used React 17 to confirm that no error is thrown.

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

Copy link
Contributor

Calling bundle size is not changed.

  • Current size: 8139366
  • Base size: 8139366
  • Diff size: 0

Copy link
Contributor

CallWithChat bundle size is not changed.

  • Current size: 8139379
  • Base size: 8139379
  • Diff size: 0

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Chat bundle size is increased❗.

  • Current size: 1757581
  • Base size: 1757580
  • Diff size: 1

Copy link
Contributor

Copy link
Contributor

github-actions bot commented Oct 24, 2024

@azure/communication-react jest test coverage for stable.

Lines Statements Functions Branches
Base 26860 / 43236
62.12%
26860 / 43236
62.12%
733 / 1342
54.61%
2136 / 3420
62.45%
Current 26855 / 43236
62.11%
26855 / 43236
62.11%
733 / 1342
54.61%
2149 / 3430
62.65%
Diff -5 / 0
-0.01%
-5 / 0
-0.01%
0 / 0
0%
13 / 10
0.2%

Copy link
Contributor

github-actions bot commented Oct 24, 2024

@azure/communication-react jest test coverage for beta.

Lines Statements Functions Branches
Base 53848 / 88045
61.15%
53848 / 88045
61.15%
1100 / 2488
44.21%
3222 / 5331
60.43%
Current 53838 / 88045
61.14%
53838 / 88045
61.14%
1100 / 2488
44.21%
3222 / 5334
60.4%
Diff -10 / 0
-0.01%
-10 / 0
-0.01%
0 / 0
0%
0 / 3
-0.03%

@mgamis-msft
Copy link
Contributor Author

We should guard our components from returning undefined. However, there is only a draft PR to enforce it for eslint-plugin-react jsx-eslint/eslint-plugin-react#3750

@mgamis-msft mgamis-msft enabled auto-merge (squash) October 24, 2024 22:16
Copy link
Contributor

Copy link
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

2 similar comments
Copy link
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

Copy link
Contributor

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot.

@mgamis-msft mgamis-msft merged commit df82c45 into main Oct 25, 2024
39 checks passed
@mgamis-msft mgamis-msft deleted the mgamis/breakout-rooms-banner-fix-for-react-17-and-lower branch October 25, 2024 02:13
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.

3 participants