Display correct guidance text for device permission error on safari by carocao-msft · Pull Request #2590 · Azure/communication-ui-library

carocao-msft · 2022-12-13T18:14:11Z

What

Display correct guidance text for device permission error on safari

Why

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

How Tested

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 · 2022-12-13T18:23:03Z

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

github-actions · 2022-12-13T18:23:43Z

Chat bundle size is increased❗.

Current size: 5577063
Base size: 5575150
Diff size: 1913

github-actions · 2022-12-13T18:23:44Z

Calling bundle size is increased❗.

Current size: 5483538
Base size: 5481071
Diff size: 2467

github-actions · 2022-12-13T18:23:54Z

CallWithChat bundle size is increased❗.

Current size: 5871398
Base size: 5868931
Diff size: 2467

github-actions · 2022-12-13T18:33:35Z

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

github-actions · 2022-12-13T20:30:05Z

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

…2585) * Add continue button to browserVersion UI * update styles * add new button logic to call adapter * add logic to callWithChat * fix handler logic * fix styles * Fix cc and build stable API * remove console * Change files * Duplicate change files for beta release * update strings * update styles for strings * update per comments * update UI state naming * build API * fix test errors * Update packages/react-composites CallComposite browser test snapshots * Update packages/react-composites CallComposite browser test snapshots * update styles * Update packages/react-composites CallComposite browser test snapshots * update styles * Update packages/react-composites CallComposite browser test snapshots * Add new tests * Change files * Duplicate change files for beta release * Update packages/react-composites CallComposite browser test snapshots * fix styles and storybook bug * Update packages/react-composites CallComposite browser test snapshots Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…fulCallClientState (#2593) * update adapted selector function * Change files * Duplicate change files for beta release * remove console

github-actions · 2022-12-14T23:30:34Z

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

github-actions · 2022-12-14T23:36:03Z

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

github-actions · 2022-12-15T00:17:19Z

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

github-actions · 2022-12-15T00:30:58Z

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

github-actions · 2022-12-15T01:14:33Z

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

github-actions · 2022-12-15T01:59:36Z

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

JamesBurnside · 2022-12-15T02:15:55Z

+  /**
+   * Whether we are using safari browser or not
+   */
+  isSafari?: boolean;


isSafari is OK, but if we ever needed to do something specific for firefox having a boolean makes it less extensible

If we want to move the logic to callreadinessmodal and pass it down to domain permissions, we can only pass along a boolean. Earlier I had the logic inside domain permissions component so that when we need to do something for firefox it also works

Why is it we can only pass a boolean? We can do

const isSafari = _isSafari(environmentInfo); <CameraAndMicrophoneDomainPermissions browser={isSafari ? 'safari' | 'other'} />

In this way we are still limited when we need to do something specific for firefox, since 'other' includes chrome and firefox and edge. Should we have the browser type to be safari | firefox | chrome | other ? But then our logic here doesn't make sense browser={isSafari ? 'safari' | 'other'} since we are assigning chrome and firefox to other.

When we need to do firefox we can then add it, but you're correct that other isn't so great, perhaps:
browserHint: 'safari' | 'unset'
?

So I like the idea of spreading it out to define the major browsers explicitly so other refers to browsers like Brave, duckduckgo and so on. I'd argue Opera might be in there too but it is prominent enough in the world that Calling could want to support it in the distant future.

this would also give a vector to have the component know which browser its on in the future. (thinking of maybe that gif thing Mikhail mentioned this morning in the review)

JamesBurnside

We can iterate on the isSafari prop in future so not blocking for this PR, at GA we will likely get feedback from review boards to avoid the boolean - we can always update then - otherwise Pr looks great!

carocao-msft added 3 commits December 13, 2022 10:11

correct guidance text safari

028e423

Change files

e9e32dd

Duplicate change files for beta release

216b3fd

carocao-msft assigned JamesBurnside Dec 13, 2022

carocao-msft requested review from alkwa-msft, anjulgarg and dmceachernmsft as code owners December 13, 2022 18:14

carocao-msft assigned edwardlee-msft Dec 13, 2022

carocao-msft requested review from JamesBurnside, PorterNan, edwardlee-msft, mgamis-msft and prabhjot-msft as code owners December 13, 2022 18:14

Merge branch 'main' into carocao/safariCallReadiness

25be531

correct guidance text safari

50fd582

JamesBurnside reviewed Dec 13, 2022

View reviewed changes

Comment thread packages/react-components/src/components/DevicePermissions/DomainPermissions.tsx Outdated

correct guidance text safari

3443258

carocao-msft assigned dmceachernmsft and edwardlee-msft and unassigned edwardlee-msft and dmceachernmsft Dec 13, 2022

carocao-msft changed the title ~~Display correct guidance text for device permission error on safari~~ Display correct guidance text for device permission error on safari [in progress] Dec 14, 2022

dmceachernmsft mentioned this pull request Dec 14, 2022

[CallReadiness] Update composite useAdapted selector to get new StatefulCallClientState #2593

Merged

dmceachernmsft and others added 2 commits December 14, 2022 14:51

[CallReadiness] Update composite useAdapted selector to get new State…

d069520

…fulCallClientState (#2593) * update adapted selector function * Change files * Duplicate change files for beta release * remove console

use environment info

e084386

carocao-msft changed the title ~~Display correct guidance text for device permission error on safari [in progress]~~ Display correct guidance text for device permission error on safari Dec 14, 2022

carocao-msft requested a review from JamesBurnside December 14, 2022 22:51

carocao-msft and others added 3 commits December 14, 2022 14:51

Merge branch 'main' into carocao/safariCallReadiness

e096bce

build files

aef418f

build files

5d104f9

dmceachernmsft reviewed Dec 14, 2022

View reviewed changes

Comment thread change-beta/@azure-communication-react-60233904-ae91-42c7-9b22-9394d4780adb.json

Comment thread packages/react-components/src/components/DevicePermissions/DomainPermissions.tsx Outdated

pr change

b0a8306

dmceachernmsft reviewed Dec 15, 2022

View reviewed changes

Comment thread packages/react-composites/src/composites/CallComposite/components/CallReadinessModal.tsx

Merge branch 'main' into carocao/safariCallReadiness

fe138a9

JamesBurnside reviewed Dec 15, 2022

View reviewed changes

Comment thread packages/react-components/src/components/DevicePermissions/DomainPermissions.tsx Outdated

JamesBurnside reviewed Dec 15, 2022

View reviewed changes

Comment thread packages/react-components/src/components/DevicePermissions/DomainPermissions.tsx Outdated

JamesBurnside reviewed Dec 15, 2022

View reviewed changes

Comment thread packages/react-components/src/components/DevicePermissions/DomainPermissions.tsx Outdated

pr change

5f61505

carocao-msft requested review from JamesBurnside and dmceachernmsft December 15, 2022 01:06

pr change

5bb73ba

pr change

8e6a9f5

JamesBurnside reviewed Dec 15, 2022

View reviewed changes

JamesBurnside approved these changes Dec 15, 2022

View reviewed changes

dmceachernmsft approved these changes Dec 15, 2022

View reviewed changes

carocao-msft merged commit b658baf into main Dec 15, 2022

carocao-msft deleted the carocao/safariCallReadiness branch December 15, 2022 21:51

Conversation

carocao-msft commented Dec 13, 2022

What

Why

How Tested

Process & policy checklist

Uh oh!

github-actions Bot commented Dec 13, 2022

Uh oh!

github-actions Bot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Chat bundle size is increased❗.

Uh oh!

github-actions Bot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Calling bundle size is increased❗.

Uh oh!

github-actions Bot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CallWithChat bundle size is increased❗.

Uh oh!

github-actions Bot commented Dec 13, 2022

Uh oh!

Uh oh!

github-actions Bot commented Dec 13, 2022

Uh oh!

github-actions Bot commented Dec 14, 2022

Uh oh!

github-actions Bot commented Dec 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Dec 15, 2022

Uh oh!

github-actions Bot commented Dec 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Dec 15, 2022

Uh oh!

github-actions Bot commented Dec 15, 2022

Uh oh!

JamesBurnside Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

carocao-msft Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

JamesBurnside Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

carocao-msft Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesBurnside Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

dmceachernmsft Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesBurnside left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Dec 13, 2022 •

edited

Loading

github-actions Bot commented Dec 13, 2022 •

edited

Loading

github-actions Bot commented Dec 13, 2022 •

edited

Loading

carocao-msft Dec 15, 2022 •

edited

Loading

dmceachernmsft Dec 15, 2022 •

edited

Loading