Page MenuHomePhabricator

[native] Fix wrong title during thread creation
ClosedPublic

Authored by michal on Sep 23 2022, 5:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 3:33 AM
Unknown Object (File)
Thu, Nov 14, 8:49 PM
Unknown Object (File)
Sat, Nov 9, 3:18 PM
Unknown Object (File)
Sat, Nov 9, 7:21 AM
Unknown Object (File)
Oct 20 2024, 6:42 PM
Unknown Object (File)
Oct 15 2024, 12:11 AM
Unknown Object (File)
Oct 13 2024, 7:57 PM
Unknown Object (File)
Oct 12 2024, 12:20 AM
Subscribers

Details

Summary

ENG-1875
If you start creating a new chat and don't add any people the title will display your name and clicking it will navigate you to your private chat's settings. This happens because Ashoat is added to every chat be default and the isSearchEmpty is always false.

Test Plan

Start creating a new chat and don't select any other users, title should be "New Message". Also, test as Ashoat.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 23 2022, 5:19 AM
Harbormaster failed remote builds in B12399: Diff 17018!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 23 2022, 5:34 AM
Harbormaster failed remote builds in B12400: Diff 17019!
ashoat requested changes to this revision.Sep 23 2022, 6:40 AM
ashoat added inline comments.
native/chat/message-list-header-title.react.js
29 ↗(On Diff #17021)

Won't this break the experience for me? I think this.props.threadInfo.members.length === 1 for me...

This revision now requires changes to proceed.Sep 23 2022, 6:40 AM
native/chat/message-list-header-title.react.js
29 ↗(On Diff #17021)

To get this right you're going to have to figure out a way to filter me out of the list... I think the hack that ENG-55 references had to solve a similar problem, so there's probably some function in the codebase you can use

ashoat added inline comments.
native/chat/message-list-header-title.react.js
29 ↗(On Diff #17021)

Yeah, @varun solved this as his second task on the project actually (D2546). I think we should first extract that functionality into a separate function you can reuse, and then you can use it here. We should also consider how this works on web... does it have the same problem?

Updated the diff to use extracted method (I'm not sure if threadMembersWithoutAddedAshoat is a good name but I couldn't think of a better one...), thanks for the help!
Also, would it be better to do this in a separate diff? I'm still not sure when should I split things into other diffs.

Looks great! Please address the following two comments before landing:

  1. Can you amend the test plan to include testing the app as me to make sure this doesn't break my experience? The reason is that I'm unclear as to whether I would end up being filtered out of a pending thread when I am the viewer. (I assume I have a member.role in that scenario, but I am not sure.)
  2. Can you update ENG-281 to cover the changes introduced here? You should probably change the title, and amend the description to cover both the history (eg. intial use case, and then this use case, with diffs linked) as well as what needs to be done (threadMembersWithoutAddedAshoat should be deprecated).
This revision is now accepted and ready to land.Sep 26 2022, 4:47 AM

Additional info to the previous comment: it doesn't happen on web because when there are no users selected, the search fills the whole screen and no title is displayed.

Run CI, tested logged in as Ashoat