Video 5756 browser check #9
Conversation
| @@ -272,5 +273,22 @@ describe('the MainContent component', () => { | |||
|
|
|||
| expect(wrapper.find('div').at(1).prop('className')).toContain('hideAfter'); | |||
| }); | |||
|
|
|||
| it('should hide the following item when browser is unsupported and active pane is BrowserTest', () => { | |||
| jest.mock('twilio-video', () => ({ version: '1.2' })); | |||
There was a problem hiding this comment.
Here, jest.mock doesn't work as we expect. These jest.mock statements are automatically hoisted to the top of their scope when the tests run. Normally, they are hoisted to the very top of the file, before the import statements. This is what makes them work - the dependency is mocked before it's imported. Here, the mock is hoisted to the top of the it() block, so we're mocking twilio-video after the original is already imported. There's some good info in this thread: jestjs/jest#2582
All that said, it looks like we don't even need to mock twilio-video. Seems like Video.isSupported = false; is enough. So I think we can get rid of line 278. Same goes for Maincontent.test.tsx and BrowserTest.test.tsx.
There was a problem hiding this comment.
good to know! thanks for sharing 👍
| paper: { | ||
| padding: '1.5em', | ||
| borderRadius: '8px', | ||
| width: '388px', |
There was a problem hiding this comment.
Looks like 388px is used in three different spots. I'd say this should be moved to the theme, but I think that this hardcoded pixel value won't work when we add the responsive layout. I think this can stay the same for now, but it's something to think about when we build the responsive designs.
| Oh no, your browser isn't supported! Download and install a new one from the list, copy this url and paste | ||
| it into your new browser to restart the test. | ||
| </Typography> | ||
| <TextField |
There was a problem hiding this comment.
This matches the designs nicely, but I do have a question for Corrine: Should this field be wider?
This is what the field looks like when the app is deployed to https://rtc-diagnostics-video-zheyb5a8-1552-dev.twil.io.
Just seems like we could use more space to show the URL.
There was a problem hiding this comment.
per Corinne in slack, we'll make the text field width 100% (as wide as parent container).
| const appURL = `${window.location.href}`; | ||
|
|
||
| const copyAppLink = () => { | ||
| navigator.clipboard.writeText(appURL); | ||
| }; |
There was a problem hiding this comment.
appURL doesn't seem to do much. We could probably just do this:
| const appURL = `${window.location.href}`; | |
| const copyAppLink = () => { | |
| navigator.clipboard.writeText(appURL); | |
| }; | |
| const copyAppLink = () => { | |
| navigator.clipboard.writeText(window.location.href); | |
| }; |
There was a problem hiding this comment.
kept appURL but remove the unnecessary template literal
|
|
||
| describe('the UnsupportedBrowser component', () => { | ||
| it('should render correctly', () => { | ||
| const wrapper = mount(<UnsupportedBrowser />); |
There was a problem hiding this comment.
mount produces a huge shapshot. Can we use shallow here?`
Contributing to Twilio
Pull Request Details
JIRA link(s):
Description
This PR adds the BrowserTest pane to the Video Diagnostics App. We used the UAParser library to get the user's Browser and OS information. Note, the
.getResult()method from the library provides a lot of information about the user agent and I figured that we could include the returned object in the final results JSON object (I'll tackle this part in a future PR). If you have a different approach let me know!Also, I included some super small missing tests for action types
set-audio-input-test-reportandset-audio-output-test-reportinAppStateProvider.test.tsx.Supported Browser
Unsupported Browser
Burndown
Before review
npm testBefore merge