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

[MM-57250] Add spinner animation to start/join call button #684

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Conversation

streamer45
Copy link
Collaborator

Summary

PR implements a spinner animation in the start/join button in channel header when connecting to a call. I also updated the call widget to be consistent with this behaviour.

@abhijit-singh One detail to note is that I have not implemented a different message based on whether the user is starting or joining a call. This is because it's tricky to track that state properly. If we were to do it, the button text would end up flickering from Starting call… to Joining call… because in reality there are two connections behind the process and the one controlling the messaging is the quickest to be made (websocket). So I defaulted to the more generic Joining call…. Please let me know if there are concerns of if you find it to be a good enough compromise.

Designs at https://www.figma.com/file/UUuEi45kKku8hq5JQA7tDC/MM-57250-Provide-feedback-after-user-clicks-start-call?type=design&node-id=1-7&mode=design&t=MXQydycXqdZnW7Iw-0

Video

joining.mp4

Ticket Link

https://mattermost.atlassian.net/browse/MM-57250

@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Apr 9, 2024
@streamer45 streamer45 self-assigned this Apr 9, 2024
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks!

I put as a comment because I tried on desktop, but it doesn't seem to be working...? (maybe it's something on my end?)

@streamer45
Copy link
Collaborator Author

This is awesome, thanks!

I put as a comment because I tried on desktop, but it doesn't seem to be working...? (maybe it's something on my end?)

Let me check. I had to port the changes to global widget but only tested it from a browser.

@streamer45
Copy link
Collaborator Author

Oh I guess it's the button that needs addressing on Desktop because it won't be in sync.

@streamer45 streamer45 requested a review from cpoile April 10, 2024 15:37
@streamer45
Copy link
Collaborator Author

Thanks @cpoile , a clear oversight. Please give it a try again.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Awesome, works perfectly.
I like the Joining call... even when starting. Makes it feel like the call was started on the server and now we're joining it.

@streamer45 streamer45 added this to the v0.27.0 / MM 9.9 milestone Apr 11, 2024
@streamer45 streamer45 removed the 2: Dev Review Requires review by a core committer label Apr 11, 2024
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Looks good @streamer45!

Would it be feasible to make the error message a little more specific in case the connection times out and the call fails? Here's what it could be:
image

@streamer45
Copy link
Collaborator Author

@abhijit-singh Yes that should be doable although I'd still retain the link to the troubleshooting section.

@streamer45
Copy link
Collaborator Author

Added a new error case (and message) to show in case the connection timed out.

image

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by ux labels Apr 16, 2024
@streamer45 streamer45 merged commit b4b417e into main Apr 16, 2024
7 checks passed
@streamer45 streamer45 deleted the MM-57250 branch April 16, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants