Skip to content

[Bugfix] Bound CallReadiness screens to the boundaries of the container#2624

Merged
edwardlee-msft merged 13 commits intomainfrom
elee/bugfix-call-readiness-permission-notification-overlay
Jan 5, 2023
Merged

[Bugfix] Bound CallReadiness screens to the boundaries of the container#2624
edwardlee-msft merged 13 commits intomainfrom
elee/bugfix-call-readiness-permission-notification-overlay

Conversation

@edwardlee-msft
Copy link
Copy Markdown
Contributor

@edwardlee-msft edwardlee-msft commented Jan 3, 2023

What

Use ModalClone and a Layerhost instead of FluentUi provided Modal.
image

Why

Modal takes up full width and height of body and not the container it's called within.
https://skype.visualstudio.com/SPOOL/_workitems/edit/3086034
image

How Tested

Locally on MacOS Safari

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023 •
edited
Loading

CallWithChat bundle size is increased❗.

  • Current size: 5876835
  • Base size: 5876267
  • Diff size: 568

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023 •
edited
Loading

Calling bundle size is increased❗.

  • Current size: 5491954
  • Base size: 5491386
  • Diff size: 568

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023 •
edited
Loading

Chat bundle size is not changed.

  • Current size: 5582325
  • Base size: 5582325
  • Diff size: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-cuoekrahmt.chromatic.com/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023

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.

@edwardlee-msft
Copy link
Copy Markdown
Contributor Author

edwardlee-msft commented Jan 3, 2023 •
edited
Loading

Note that the changes include an "align-items" prop which causes the width of the modal to be a bit thinner than before:

Disregard image and comment as new implementation does not impact size.

@edwardlee-msft edwardlee-msft added the update_snapshots Set this label to request automated update of UI snapshots label Jan 3, 2023
@github-actions github-actions Bot removed the update_snapshots Set this label to request automated update of UI snapshots label Jan 3, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 3, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-tyuomvxmqh.chromatic.com/

JamesBurnside
JamesBurnside reviewed Jan 4, 2023
View reviewed changes
Comment thread packages/react-composites/src/composites/CallComposite/pages/ConfigurationPage.tsx Outdated
const [audioState, setAudioState] = useState<PermissionState | 'unsupported' | undefined>(undefined);
/* @conditional-compile-remove(call-readiness) */
getDevicePermissionState(setVideoState, setAudioState);
const modalLayerHostId = useId('callReadinessModalLayerhost');
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Jan 4, 2023

Choose a reason for hiding this comment

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

We already have a composite-root level modal layer host id, can we use this instead so all modals, context menus etc. going forward all bind to the same root:

communication-ui-library/packages/react-composites/src/composites/CallComposite/CallComposite.tsx

Line 377 in d7d036e

const modalLayerHostId = useId('modalLayerhost');

Copy link
Copy Markdown
Contributor Author

@edwardlee-msft edwardlee-msft Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea, I confirmed with FluentUi modal examples that multiple modals use 1 (the same) root modal layer host.
I'll make the change now!

Copy link
Copy Markdown
Contributor Author

@edwardlee-msft edwardlee-msft Jan 4, 2023

Choose a reason for hiding this comment

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

Alright! Done, needed to play around with some transform in the modal but got it to work with the current layerhost.
Also removed mobileView only for that specific layerhost as I found that it was not needed during my testing with the PIP modal. (but needs to be removed for call readiness modal to render on desktop using that layerhost).

@edwardlee-msft edwardlee-msft added the update_snapshots Set this label to request automated update of UI snapshots label Jan 4, 2023
@github-actions github-actions Bot removed the update_snapshots Set this label to request automated update of UI snapshots label Jan 4, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 4, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-wwghhygzej.chromatic.com/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 4, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-oxsfmduyai.chromatic.com/

@edwardlee-msft edwardlee-msft added the update_snapshots Set this label to request automated update of UI snapshots label Jan 4, 2023
@github-actions github-actions Bot removed the update_snapshots Set this label to request automated update of UI snapshots label Jan 4, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 4, 2023

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-pmydaomlyc.chromatic.com/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-gdljgcgsck.chromatic.com/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2023

Failed to pass the composite 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2023

Storybook URL https://60c7ae6891f0e90039d7cd54-jfysvyibde.chromatic.com/

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2023

Failed to pass the composite 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.

@edwardlee-msft edwardlee-msft merged commit 0b5056c into main Jan 5, 2023
@edwardlee-msft edwardlee-msft deleted the elee/bugfix-call-readiness-permission-notification-overlay branch January 5, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesBurnside JamesBurnside JamesBurnside approved these changes

@dmceachernmsft dmceachernmsft dmceachernmsft approved these changes

@alkwa-msft alkwa-msft Awaiting requested review from alkwa-msft

@anjulgarg anjulgarg Awaiting requested review from anjulgarg

@carocao-msft carocao-msft Awaiting requested review from carocao-msft

@mgamis-msft mgamis-msft Awaiting requested review from mgamis-msft

@PorterNan PorterNan Awaiting requested review from PorterNan

@prabhjot-msft prabhjot-msft Awaiting requested review from prabhjot-msft

Assignees

@JamesBurnside JamesBurnside

@dmceachernmsft dmceachernmsft

@carocao-msft carocao-msft

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@edwardlee-msft @JamesBurnside @dmceachernmsft @carocao-msft
⚡