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

fix(typing-indicator): restored indicator #4734

Closed
wants to merge 2 commits into from
Closed

Conversation

ThomBos
Copy link
Contributor

@ThomBos ThomBos commented Sep 12, 2022

What this PR does 📖

  • Restores typing indicator on the sidebar

Which issue(s) this PR fixes 🔨

Special notes for reviewers 🗒️

Additional comments 🎤

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Sep 12, 2022
@phillsatellite
Copy link
Contributor

Only 2 small things I found that I will create tickets for in the QA Jira board just wanted to share findings here as well, iOS typing indicator was appearing wrong and I noticed it didnt appear for Groupchats either but other than those 2 small things it worked extremely well! 🔥
IMG_6572
https://user-images.githubusercontent.com/93608357/189741484-fbb447cc-c4d7-48b0-b965-0b5b3e3b1596.mov

@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Sep 12, 2022
@molimauro
Copy link
Contributor

molimauro commented Sep 12, 2022

Thanks @phillsatellite! The group thing is not a bug, it’s intended :) we ll look into the ios bug though!! Thanks ;) all good on android?

)
: false
finalMask(): string {
if (this.isTyping) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be redundant

@stavares843
Copy link
Member

thanks for testing @phillsatellite 💥 thanks for clarifying @molimauro 🔨

Comment on lines +71 to +73
(this.conversation.id &&
this.ephemeral.typing[this.conversation.id][this.user.did]) ||
false
Copy link
Contributor

@nathan-power nathan-power Sep 13, 2022

Choose a reason for hiding this comment

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

Hey Thomas!
Tried adding a new friend but was met with this error. It might be good to include guard clauses here for if the conversation doesn't exist yet for the user, and if the data for the user is not yet available.
image
image

Copy link
Member

Choose a reason for hiding this comment

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

i saw this error before on dev as well

Copy link
Contributor

@nathan-power nathan-power left a comment

Choose a reason for hiding this comment

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

Hey Thomas 👋
It's looking good, just one change I noticed that might be important

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Sep 13, 2022
@josephmcg
Copy link
Contributor

#4745
Thanks for your work on this Thomas. I took some inspiration from your approach and resolved it here.

I opted for a conversationId prop to determine typing status render because the sidebarListItem component doesn't need to be aware of typing status. It was only calculating it to pass as a prop.

In that case, I think it's better to handle everything in the child component. This will also work well with a new group member sidebar I have in progress here
#4747

@josephmcg josephmcg closed this Sep 13, 2022
@stavares843 stavares843 deleted the fix-typing-sidebar branch September 22, 2022 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa QA tested One QA Person has tested - Needs QA Lead review still
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants