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

Add Reconnect UI to ConnectivityStatus when connection has been interrupted #1895

Merged
merged 13 commits into from
Apr 20, 2019

Conversation

corinagum
Copy link
Contributor

@corinagum corinagum commented Apr 12, 2019

Resolves #1523
Resolves #1831

Annotation 2019-04-11 165420

  • When Web Chat connects but then loses connection, the above component will show, indicating to user that interruption has occurred.
  • After 15 seconds, spinner animation will revert to "Connection taking longer than usual" warning icon.
  • Similar to connecting for the first time, the Network interruption animation has a short timeout to ensure it is legible for at least 0.4 of a second.
  • Web Chat users may now customize the typing indicator and spinner animation through defaultStyleSetOptions.js

@coveralls
Copy link

coveralls commented Apr 12, 2019

Coverage Status

Coverage increased (+0.3%) to 54.503% when pulling c680b25 on corinagum:1523 into 4172154 on Microsoft:master.

@compulim
Copy link
Contributor

compulim commented Apr 12, 2019

In ConnectivityStatus.js, few things need to be modified:

  • Don't put JSX outside of render() function, it makes the output very hard to read
  • Don't do side-effect during render()
    • In your render(), you call connectivityStatusText()
    • Then, in your connectivityStatusText(), you setTimeout()
    • Few reasons:
      • You never know when render() will happen
      • render() is for rendering HTML, not for business logic, need a super clean cut here
  • Line 85, this if statement (props !== state) always truthy
    • This is shallow comparison
    • Only change what you need
    • Why put styleSet in state?
  • Don't call setState() in componentDidUpdate()
    • componentDidUpdate() is being called after the screen is rendered
    • Then you call setState(), that means you are asking the screen to render again

Copy link
Contributor

@compulim compulim left a comment

Choose a reason for hiding this comment

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

As commented.

__tests__/offlineUI.js Outdated Show resolved Hide resolved
__tests__/offlineUI.js Outdated Show resolved Hide resolved
__tests__/offlineUI.js Outdated Show resolved Hide resolved
__tests__/offlineUI.js Outdated Show resolved Hide resolved
__tests__/offlineUI.js Outdated Show resolved Hide resolved
})
});

await driver.sleep(17000);
Copy link
Member

Choose a reason for hiding this comment

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

we need to wait 17 seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is waiting for the 'slow to connect' timeout, which is the 17 seconds. I'm not sure if there's any way to circumvent the timeout during tests, but if there is please let me know and I can fix it.

packages/component/src/SendBox/ConnectivityStatus.js Outdated Show resolved Hide resolved
debounce={ (connectivityStatus === 'uninitialized' || connectivityStatus === 'error') ? 0 : 400 }>
{ () =>
connectivityStatus === 'connectingslow' ?
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Are you able to render a single React.Fragment outside the chained ternary like this?

() => {
  <React.Fragment>
    {
     connectivityStatus === 'connectingslow' ?
       <WarningNotificationIcon />
     : (connectivityStatus === 'error' || connectivityStatus === 'notconnected') ?
       <ErrorNotificationIcon />
     : connectivityStatus === 'uninitialized' ?
       <SpinnerAnimation />
       { localize('INITIAL_CONNECTION_NOTIFICATION', language) }
    }
 </React.Fragment>
}

Copy link
Contributor Author

@corinagum corinagum Apr 19, 2019

Choose a reason for hiding this comment

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

I don't think this will work because each status also has different text to be localized, and you can't return multiple children, only one.

this gives errors:

  <React.Fragment>
    {
     connectivityStatus === 'connectingslow' ?
       <WarningNotificationIcon />
       { localize('SLOW_CONNECTION_NOTIFICATION', language) }
     : (connectivityStatus === 'error' || connectivityStatus === 'notconnected') ?
       <ErrorNotificationIcon />
       { localize('FAILED_CONNECTION_NOTIFICATION', language) }
     : connectivityStatus === 'uninitialized' ?
       <SpinnerAnimation />
       { localize('INITIAL_CONNECTION_NOTIFICATION', language) }
    }
 </React.Fragment>
}

Playing around with parenthesis didn't get rid of the errors. Let me know if I should try something else. :)

@corinagum
Copy link
Contributor Author

PR modifications have been pushed up, with an added fix:
#1831 - Typing Indicator and Spinner now have the ability to be specified by the user in defaultStyleSetOptions.js.

@corinagum
Copy link
Contributor Author

@cwhitten could you take another pass when you have a chance? Thank you!

@corinagum corinagum merged commit 09536a5 into microsoft:master Apr 20, 2019
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.

Add Typing Indicator background URL to defaultStyleOptions Need offline UI: Connection interrupted
5 participants