Skip to content

Callwithchat mobile: dismissed drawer after device selection#2115

Merged
carocao-msft merged 10 commits intomainfrom
carocao/BUG2896714
Jul 25, 2022
Merged

Callwithchat mobile: dismissed drawer after device selection#2115
carocao-msft merged 10 commits intomainfrom
carocao/BUG2896714

Conversation

@carocao-msft
Copy link
Copy Markdown
Contributor

@carocao-msft carocao-msft commented Jul 21, 2022

What

Callwithchat mobile: make drawer dismissed after device selection

Why

https://skype.visualstudio.com/SPOOL/_workitems/edit/2896714

Test

Screen.Recording.2022-07-22.at.11.00.58.AM.mov
Screen.Recording.2022-07-22.at.12.16.50.PM.mov

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 21, 2022

CallWithChat bundle size is increased❗.

  • Current size: 5486905
  • Base size: 5486811
  • Diff size: 94

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 21, 2022

Chat bundle size is increased❗.

  • Current size: 5263268
  • Base size: 5263245
  • Diff size: 23

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 21, 2022

Calling bundle size is increased❗.

  • Current size: 5088312
  • Base size: 5087383
  • Diff size: 929

@JamesBurnside
Copy link
Copy Markdown
Member

JamesBurnside commented Jul 21, 2022

Two things to consider:

  • The mobile drawer is also used when a participant is clicked in callwithchat - can either add the same manual close logic to when they are clicked too, or consider instead changing the _DrawerMenu itself to trigger close when a menu item (that doesn't have a sub menu) is clicked
  • We have some UI tests that I think will need the test checks updated to reopen the drawer if the drawer dismisses on click

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@carocao-msft
Copy link
Copy Markdown
Contributor Author

carocao-msft commented Jul 22, 2022

Two things to consider:

  • The mobile drawer is also used when a participant is clicked in callwithchat - can either add the same manual close logic to when they are clicked too, or consider instead changing the _DrawerMenu itself to trigger close when a menu item (that doesn't have a sub menu) is clicked
  • We have some UI tests that I think will need the test checks updated to reopen the drawer if the drawer dismisses on click

Updated PR with screen recording , verified both participant button and audio/camera selection drawers are dismissed after selection

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

props.onLightDismiss();
},
[speakers, onSelectSpeaker]
[speakers, onSelectSpeaker, props]
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Jul 22, 2022

Choose a reason for hiding this comment

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

nit: avoid props as this means any prop change will cause this to be recreated.
Instead update the above const { speakers, onSelectSpeaker } = props;
to have the onLightDismiss:
const { speakers, onSelectSpeaker, onLightDismiss } = props;
and put onLightDismiss into the dependency array

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^ I second this as well!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated :)

props.onLightDismiss();
},
[microphones, onSelectMicrophone]
[microphones, onSelectMicrophone, props]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(nit: same here on avoiding props in the dep array)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^ I second this as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated :)

props.onLightDismiss();
},
[microphones, onSelectMicrophone]
[microphones, onSelectMicrophone, props]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^ I second this as well

props.onLightDismiss();
},
[speakers, onSelectSpeaker]
[speakers, onSelectSpeaker, props]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

^^ I second this as well!

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

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

@carocao-msft carocao-msft enabled auto-merge (squash) July 25, 2022 16:32
@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

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

2 similar comments
@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

@carocao-msft carocao-msft merged commit 8c74438 into main Jul 25, 2022
@carocao-msft carocao-msft deleted the carocao/BUG2896714 branch July 25, 2022 18:08
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