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.

Sign up for GitHub

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

Jump to bottom

fix(replies): show replies and improve UX related to sending a reply - AP-124 #654

Merged
merged 4 commits into from
Dec 17, 2021
Merged

fix(replies): show replies and improve UX related to sending a reply - AP-124 #654

merged 4 commits into from
Dec 17, 2021

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Dec 15, 2021 •
edited by stavares843
Loading

What this PR does 📖

  • Displays replies in chat
  • remove blue reply banner while message is sending
  • clear chatbar after reply has been sent
  • fix file message encoder to account for jeffs update
  • restructure message encoders (with guidance from Manuel)

Which issue(s) this PR fixes 🔨

Fixes # AP-124, AP-243

Special notes for reviewers 🗒️

Additional comments 🎤
I noticed three bugs that are somewhat tricky and maybe better suited for a separate issue:

  • emoji reactions are applied to every reply of a message.
  • attempting to emoji react to a reply crashes
  • you can only reply to the first message of a group

@josephmcg josephmcg added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Dec 15, 2021
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Dec 16, 2021
@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

2 similar comments
@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@github-actions
Copy link
Contributor

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@josephmcg josephmcg changed the title AP124 - Chat- Replies are not appearing in chat fix(replies): show replies and improve UX related to sending a reply Dec 16, 2021
@josephmcg josephmcg removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Dec 16, 2021
@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Dec 16, 2021
@stavares843
Copy link
Member

pretty neat! 🎉

@stavares843
Copy link
Member

also added tickets for what you mentioned above

AP-289
AP-290
AP-291

@stavares843 stavares843 added temporary blocked checking something QA Lead is checking something. and removed Ready for QA Ready for QA team to test, Devs approved. labels Dec 16, 2021
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Dec 16, 2021
@stavares843 stavares843 changed the title fix(replies): show replies and improve UX related to sending a reply fix(replies): show replies and improve UX related to sending a reply - AP-124 Dec 17, 2021
Matt Wisniewski and others added 3 commits December 17, 2021 10:13
@josephmcg
rebase from dev
60ca9c1
@josephmcg
fix(reply): display reply of type text
d67dc43 
*changed reply structure with advice from Manuel
*added conditions for rendering different types of replies. text only for now
@josephmcg
fix(replies): improve reply process
a4a88c6 
* remove blue reply banner while message is sending
* clear chatbar after reply has been sent
* fix file message encoder to account for jeffs update
@josephmcg josephmcg removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Dec 17, 2021
@josephmcg
Copy link
Contributor Author

conflict was created here https://github.com/Satellite-im/Core-PWA/pull/614/files
components/views/chat/message/reply/Reply.html

it was updated using old component names that no longer exist. It was difficult to see because those if conditions would never evaluate true.

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed temporary blocked checking something QA Lead is checking something. labels Dec 17, 2021
@stavares843
Copy link
Member

neat! 🎉

@stavares843 stavares843 merged commit f54c718 into dev Dec 17, 2021
@stavares843 stavares843 deleted the AP124 branch December 17, 2021 01:55
@github-actions github-actions bot removed the Ready for QA Ready for QA team to test, Devs approved. label Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WanderingHogan WanderingHogan WanderingHogan approved these changes

@InfamousVague InfamousVague Awaiting requested review from InfamousVague

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@josephmcg @stavares843 @InfamousVague @WanderingHogan