Happychat: Refactor out helper.js for clearer loading states #12706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matticbot
added
the
[Size] XL
mattwondra
added
Happychat
[Status] Needs Review
mattwondra
force-pushed
the
update/happychat/refactor-helpers
branch
from
e92a9cf to
3cca632
Compare
mattwondra
changed the base branch from
master
to
fix/happychat/better-connect-redux-lifecycle
jordwest
approved these changes
Apr 5, 2017
| @@ -63,7 +72,10 @@ export const Composer = React.createClass( { | |||
| } | |||
| } ); | |||
|
|
|||
| const mapState = ( { happychat: { message } } ) => ( { message } ); | |||
| const mapState = state => ( { | |||
| disabled: getHappychatConnectionStatus( state ) !== 'connected', | |||
There was a problem hiding this comment.
I'm wondering if we should extract this into a selector, for example canUserSendMessage?
We might also want to check the chat status as well, for example if the chat is connected but pending or missed.
There was a problem hiding this comment.
Great suggestion, I'll add that in.
Yep, that's done in PR #12754. Both of these PRs are based on your work in #12219, but I just split them up so that the review of the helper.js refactor and the disconnect messaging were both a little easier. |
mattwondra
force-pushed
the
update/happychat/refactor-helpers
branch
from
3cca632 to
8de92f3
Compare
mattwondra
changed the base branch from
fix/happychat/better-connect-redux-lifecycle
to
master
matticbot
removed
the
[Status] Needs Review
This was referenced Apr 5, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR lays foundation for improving Happychat connection statuses to handle lost connections and reconnects (#12183), by refactoring out the
helper.jswhich primarily existed to show interface elements in various states. Most of this work comes from @jordwest's PR #12219, which I'm splitting up into two parts — the next PR will introduce new connection statuses for various connectivity issues.The screenshots below compare the current Happychat load experience with the new one. This shows on the
/me/chatpage where HC is visible from the beginning, but in practice most users will interact through the sidebar HC. That sidebar usually pre-connects before it's visible, so most of the time users won't see these loading states. But simplifying in this way will help with the following PR, where we'll give users more feedback about connection issues.Screenshot comparison
Current experience
Uninitialized:
Connecting:
Connected:
New experience
Uninitialized:
Connecting:
Connected:
To test
Make sure that normal paths to Happychat communication work as expected. You can use Redux Dev Tools to scrub through the various loading states.