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

feat(WEBRTC-317): add methods to check webrtc compatibility and list the supported browser #135

Merged
merged 34 commits into from
Jan 21, 2021

Conversation

DeividVeloso
Copy link
Contributor

  • add a new method to check if the current browser has supported to use TelnyxRTC
  • add a new method to list supported system and browser by TelnyxRTC
  • add type docs for the above methods
  • add unit test for the above methods

📝 To Do

  • All linters pass
  • All tests pass
  • Change documentation based on my changes

✋ Manual testing

  1. Navigate into packages/js and run npm run build
  2. Navigate into js/examples/react-audio and run npm setup
  3. Access http://localhost:6006/?path=/story/webdialer--example and open the inspect browser
  4. Open console and type this TelnyxRTC.webRTCInfo() to know if the browser supports TelnyxRTC
  5. Open console and type this TelnyxRTC.webRTCSupportedBrowserList() to know the supported browsers
  6. You should test in different browsers and see the results.
  7. Navigate into packages/js and npm run test you should get all green.
  8. Navigate into packages/js and npm run docs you should see the new methods documented on TelnyxRTC.MD

🦊 Browser testing

Desktop

  • Edge (latest)
  • Chrome
  • Firefox
  • Safari
  • Opera

📸 Screenshots

Supported browser result

image

Not supported browser

image

@DeividVeloso DeividVeloso requested a review from SuaYoo January 19, 2021 20:33
Copy link
Contributor

@SuaYoo SuaYoo left a comment

Choose a reason for hiding this comment

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

General question, do we have internal documentation on how we determine browser support? I'm curious as to why we've defined a custom list of supported browsers. Any specific reason not to use something like https://www.npmjs.com/package/detectrtc?

@DeividVeloso
Copy link
Contributor Author

General question, do we have internal documentation on how we determine browser support? I'm curious as to why we've defined a custom list of supported browsers. Any specific reason not to use something like https://www.npmjs.com/package/detectrtc?

1 - about browser support, I defined those major browsers that I've tested and worked. For example Firefox we are supporting partially because of the plan-b. it can change when we define unified-plan and when we add more integration tests we can increase the support list.

2 - about detectrtc I didn't use any third library to avoid create dependency for small things like check if the browser has RTCPerrConnection implemented.

I'm open to change if you think it is better, what do you think?

@DeividVeloso DeividVeloso requested a review from SuaYoo January 20, 2021 15:20
@DeividVeloso DeividVeloso requested a review from SuaYoo January 20, 2021 17:54
@DeividVeloso DeividVeloso merged commit de3d3c7 into main Jan 21, 2021
@DeividVeloso DeividVeloso deleted the WEBRTC-317 branch January 21, 2021 21:10

```js
const info = TelnyxRTC.webRTCInfo();
const isWebRTCSupported = info.supportWebRTC;
Copy link
Contributor

Choose a reason for hiding this comment

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

info.supportsWebRTC

Comment on lines +1000 to +1007
| | Chrome | Firefox | Safari | Edge |
|---------|--------|---------|--------|------|
| Android | [-] | [-] | [ ] | [ ] |
| iOS | [ ] | [ ] | [-] | [ ] |
| Linux | [x] | [-] | [ ] | [ ] |
| MacOS | [x] | [-] | [x] | [-] |
| Windows | [x] | [-] | [ ] | [-] |

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to use

-- instead of [ ] for browsers that do not exist (Safari on Linux)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants