Order by most recent along with report icons by chiragsalian · Pull Request #759 · Expensify/App

chiragsalian · 2020-11-05T02:04:06Z

~~HOLD for Web-E PR to go live.~~
~~HOLD for Web-E PR to go live~~
HOLD for Web-E PR to go live.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/145238

Tests

Pull in this web-E PR.
Open expensify chat. Click through a few of your reports. Remember the order of reports you clicked through if possible.
Click on the search bar. Confirm you see your reports sorted in the same order.
Confirm 1:1 DMs show up with Add button and group DMs don't have add button.
Click on a name and confirm that report opens up.
Click search again and type out a name. This is not sorted by last visited but we confirm that nothing is broken here.
Select a name or add to group chat and confirm nothing is broken.
Confirm in the LHN, recently visited chats or while chat searching that any user that has an icon shows their icons.

Screenshots

Pinned chats (the only difference is that icons now show up here as well)

Pinned chats mobile

Empty chat search showing recently visited chats - web

Empty chat search showing recently visited chats - mobile

Searching for a person - web

Searching for a person - mobile

shawnborton · 2020-11-11T00:39:46Z

Can you share screenshots of what you are working on?

chiragsalian · 2020-11-11T01:02:35Z

Yup for sure. Just about finishing up the code. For context - This is to show the most recent visited chats when the search bar is empty as mentioned in this design doc here

I've gone a little ahead of myself and was able to link icons for each report based on the user avatars. To which i can even apply for group DMs a max of 3 icons though. This is a first version of the UI and i thought i'd get your opinions if we'd want to improve on it now or later.

Anyway UI for web,
Pinned chats (the only difference is that icons now show up here as well)

Pinned chats mobile

Empty chat search showing recently visited chats - web

Empty chat search showing recently visited chats - mobile

Searching for a person - web

Searching for a person - mobile

shawnborton · 2020-11-11T01:05:33Z

Got it - were there any designs by the design team that you had been working off of, or was this covered in a design doc somewhere? Just curious as this all seems super new to me, and I don't recall discussing it anywhere.

The three avatars in a row feels odd to me. I think if anything, we were toying around with mimicking Slack and showing just two avatars in the row, which would occupy the same amount of space as an avatar in a chat row:

chiragsalian · 2020-11-11T01:26:36Z

The only design i used once the one posted in this design doc. i.e.,

which is mostly about sorting chats and not the icons. Basically in my pursuit to sort by recent i was able to link icons to reports and hence wanted to display them. The hard part for me was mostly getting those icons such that they can always be display quickly. Now that we have the information i'm completely open to any icon improvements you have. Would you like to work off my PR and update the styles for the icons? If you are you'll need to pull in this Web-E PR as well.

shawnborton · 2020-11-11T01:38:24Z

Well I would suggest that you just stick to the designs found in design docs for now, and if we want to make the improvement of showing multiple avatars in the chat rows, we should put forward a quick little P/S statement in #expensify-cash and see what the wider audience thinks before just deciding here in this PR what we think it should look like. We can share some screenshots of what it might look like and get some wider buy-in first.

I think what I am really trying to say is, I do appreciate you wanting to improve the appearance of these rows, but we're not following standard practices in terms of working with the design team on exploring the UI or discussing this with a wider audience in a Design Doc/Slack/GH issue first.

chiragsalian · 2020-11-11T02:02:23Z

Yup i wasn't planning on pushing this out without getting inputs. My main challenge was just storing this information in this report. I was planning on showing icons for 1:1 DMs and hiding the icons for others. This way when a decision is made for group DMs the data is ready to be presented as we see fit 🙂 Was mostly just playing around with some things and testing a lot which is why the PR is still in draft. I should be doing my final testing tomorrow, demo it and then i'll push up the final screenshots which will not have icons for group DMs.

shawnborton · 2020-11-11T02:05:32Z

Okay that makes sense, sorry for jumping the gun here!

chiragsalian · 2020-11-11T19:22:58Z

It's all cool. No harm in getting the input earlier than later 🙂 I've updated the code and the screenshots in the issue such that only 1:1 DM have report icons. I've a little more to test before i can put this out for review.

marcaaron

LGTM. Left some NABs and thoughts.

While testing I found a few issues:

Deleting last text character in search box closes the switcher and causes the input to lose focus
Default avatars are not being used for search results despite existing.
Some chat links are linking to r/undefined (does not affect ability to create chat) and weirdly these ones DO have the default avatars

marcaaron · 2020-12-08T18:40:52Z

+            'reportNameValuePairs',
+            `lastRead_${currentUserAccountID}`,
+            'timestamp'
+        ], 0)


NAB not sure if it's very valuable but a utility that let us do

const lastRead = getLastRead(report, accountID); console.log(lastRead.sequenceNumber); console.log(lastRead.timestamp);

might help us DRY things up a little instead of having to use lodashGet

Yeah i don't see much value in a utility function like so when it can be directly accessed in lodashGet. Like atm i don't see a point of returning an object than the required value directly.

marcaaron · 2020-12-08T18:58:20Z

            // Store only the absolute bare minimum of data in Onyx because space is limited
            const newReport = getSimplifiedReportObject(report);
            newReport.reportName = getChatReportName(report.sharedReportList);
+            newReport.lastVisitedTimestamp = Date.now();


NAB/possible follow up... one thought I'm having is that we could also update the lastRead_ NVP timestamp for the user who created this report when calling CreateChatReport. Then we would not need to do Date.now() but perhaps it doesn't matter. I find it slightly confusing that we would need to do this here in the client instead of refer to the NVP that we fetch immediately after creating the report.

Technically we don't even need this code because Report_UpdateLastRead is called shortly after which will set the NVP but this code is just faster so that if the user switches out of a new report quickly they'll see the change immediately. I don't think CreateChatReport needs to be updated because Report_UpdateLastRead is called shortly after though.

That makes sense to me I think. Except this part...

if the user switches out of a new report quickly they'll see the change immediately

Not sure it will be obvious to anyone scanning this code that this is the reason we are setting this on the report. How much faster does it make things? If we really want to keep it maybe we can add some context here for why it's being updated immediately.

marcaaron · 2020-12-08T18:59:50Z

                showsVerticalScrollIndicator={false}
                data={options}
-                keyExtractor={option => (option.type === 'user' ? option.alternateText : String(option.reportID))}
+                keyExtractor={option => option.keyForList}


NAB key is probably fine as we will know what it's for based on how it gets used

went with the suggestion mentioned here

marcaaron · 2020-12-08T19:07:00Z

+                return true;
+            })
+            .map((report) => {
+                const participants = lodashGet(report, 'participants', []);


NAB, not sure about defaulting to an empty array here. If participants is empty I'd think something has went wrong upstream and we would not want to show this chat.

Well i haven't come across an empty participant chat but the default param is just so that things down blow up if we did. Sorta like null coalesce in php. I could add a filter exception above to remove reports with no participants but i don't think i should because i can totally imagine reports without participants down the line. Because participants don't include the current user i can see a 1:1 with the current user (sorta like how i can talk to myself on slack) or a channel with just the one user to be permitted. Thoughts?

chiragsalian · 2020-12-08T23:04:05Z

Thanks for the testing 🙂 Updated code to address review comments.

Additionally,

Deleting last text character in search box closes the switcher and causes the input to lose focus

Fixed this.

Default avatars are not being used for search results despite existing.

I didn't realize about the default avatars. Updated the code so that default avatars show up while searching as well.

Some chat links are linking to r/undefined (does not affect ability to create chat) and weirdly these ones DO have the default avatars

Thats the same in production if you're looking up a person that you haven't chatted with before. I'm not really changing anything here since those flows will create a chat from personalDetailOptions.

tgolen · 2020-12-10T17:47:21Z

While testing this, I ran across an issue with the name of the report shown in the LHN not listing everyone properly.

It appears this is a regression because here is how it looks on master:

chiragsalian · 2020-12-10T17:53:27Z

Weird, i see it like this,

Any chance its a caching issue for you? Maybe try clearing local data, signout and signin and see if its resolved? I'll re-check after my 1:1

tgolen

OK, I know these are some pretty verbose comments, but I wanted to be sure I was providing the full context around my suggestions.

tgolen · 2020-12-10T18:20:11Z

+                    participants,
+                    icon: report.icon,
+                    login,
+                    type: isSingleUserChat ? CONST.REPORT.SINGLE_USER_CHAT : CONST.REPORT.GROUP_CHAT,


NAB I think at some point (maybe not this PR, but pretty soon) the way that options are being rendered needs to be refactored. I'm thinking that each option should have its own render method (so it can render the option however it needs to) and its own onSelect callback. These options were meant to be something generic that can just be passed to a list, but it's already outgrown it's original purpose, and this is only going to continue to get more complex. I think the LHN needs to be much more extensible to support virtually any type of option, displayed in any way, and does any kind of logic when it's selected.

So i did create a follow up issue thats on hold for this - https://github.com/Expensify/Expensify/issues/145987
That issue will focus on refactoring these options such that they are created just once than on each keytype which should speed up the app. We can discuss more improvements there as needed and i'll apply them.

I'm thinking that each option should have its own render method (so it can render the option however it needs to)

I would like to request we pause on ChatSwitcher refactors for now as we are going to soon have a New Chat and New Group as separates pages and flows as per the Consumer UI Refresh project and the "ChatSwitcher" will just become the a chat search tool that returns chats.

I think after we do that we'll have some separation between the flows and it will make things a bit easier to understand. And that doc is the perfect place to dig into the details for how it should be done. Does that sound OK?

I think the LHN needs to be much more extensible to support virtually any type of option, displayed in any way, and does any kind of logic when it's selected.

What kinds of things are you imagining we will have here?

Does that sound OK?

Yeah, that sounds great!

What kinds of things are you imagining we will have here?

I think this is going to contain lists of public reports and also policy reports, and maybe even something not related to reports at all (like a link to another page or something). It's difficult to see what more will be needed here, just like when this code was first written, it didn't consider that there would be three different types of options.

chiragsalian · 2020-12-10T21:16:33Z

Updated code to resolve comments. I've also updated the language of my code to not say "chat" and instead just keep it as "report". Re-tested the same. Ready for review.

marcaaron

LGTM and tests well. Just left one minor comment, but I don't think it's a blocker

marcaaron · 2020-12-10T22:05:17Z

            // Store only the absolute bare minimum of data in Onyx because space is limited
            const newReport = getSimplifiedReportObject(report);
            newReport.reportName = getChatReportName(report.sharedReportList);
+            newReport.lastVisitedTimestamp = Date.now();


That makes sense to me I think. Except this part...

if the user switches out of a new report quickly they'll see the change immediately

Not sure it will be obvious to anyone scanning this code that this is the reason we are setting this on the report. How much faster does it make things? If we really want to keep it maybe we can add some context here for why it's being updated immediately.

tgolen

Cool, this looks good. We worked through a bug with some default avatars not showing. I tested this on web and iOS and it works well. @marcaaron all yours.

Order by most recent along with report icons

004d79f

chiragsalian self-assigned this Nov 5, 2020

chiragsalian added 13 commits November 5, 2020 16:45

code for chat report icons

2afde47

icon fix

4e73d87

Fixes to support add to group button

35918f7

Fixes to search and ordering

4a26675

record action properly and sorting improvements

62a157d

cleanups

2da2772

remove console

2cdf9a4

Merge branch 'master' into chirag-show-most-recent

226d702

moving icon logic from switcherList to row

68b4d76

style fix

1746fc8

minor correction

5f11d83

Update as per discussion to not sort during search

78d43e8

style fix

8dc27d7

puneetlath mentioned this pull request Nov 11, 2020

Make unread chats bold in chat switcher #787

Merged

chiragsalian added 3 commits November 11, 2020 09:47

Merge branch 'master' into chirag-show-most-recent

a259d7a

single icon

6e87007

Icon and search revert to regex with some improvements

306d88a

chiragsalian changed the title ~~Order by most recent along with report icons~~ [HOLD for Web-E #29599] Order by most recent along with report icons Nov 11, 2020

chiragsalian added 2 commits November 11, 2020 11:49

cleanup

32f5988

Merge branch 'master' into chirag-show-most-recent

10b7c7b

marcaaron requested changes Dec 8, 2020

View reviewed changes

chiragsalian added 2 commits December 8, 2020 14:45

cleaning up some flows

d437b3e

moar cleanup

f5406fb

not calling updateSearch twice since the same happens in focus

a9ebd92

tgolen requested changes Dec 10, 2020

View reviewed changes

chiragsalian added 5 commits December 10, 2020 11:26

comment updates

5161b3e

Merge branch 'master' into chirag-show-most-recent

94dcd24

comment correction

4c52b72

updating language

3469012

correcting comment

5bff415

chiragsalian requested review from marcaaron and tgolen December 10, 2020 21:16

cleanup

625097f

marcaaron previously approved these changes Dec 10, 2020

View reviewed changes

adding back formatPersonalDetails that i mistakenly removed

2f5447d

chiragsalian dismissed marcaaron’s stale review via 2f5447d December 10, 2020 22:16

chiragsalian added 2 commits December 10, 2020 14:35

Fix for empty avatar icon

d1d5329

missed colon

fdb673e

tgolen previously approved these changes Dec 10, 2020

View reviewed changes

Added comment

22104c9

chiragsalian dismissed tgolen’s stale review via 22104c9 December 10, 2020 22:46

correction

022ca53

marcaaron approved these changes Dec 10, 2020

View reviewed changes

marcaaron merged commit 46f6373 into master Dec 10, 2020

marcaaron deleted the chirag-show-most-recent branch December 10, 2020 23:25

NikkiWines mentioned this pull request Dec 21, 2020

Allow users to search for chats by email #1029

Merged

Conversation

chiragsalian commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

Screenshots

Uh oh!

shawnborton commented Nov 11, 2020

Uh oh!

chiragsalian commented Nov 11, 2020

Uh oh!

shawnborton commented Nov 11, 2020

Uh oh!

chiragsalian commented Nov 11, 2020

Uh oh!

shawnborton commented Nov 11, 2020

Uh oh!

chiragsalian commented Nov 11, 2020

Uh oh!

shawnborton commented Nov 11, 2020

Uh oh!

chiragsalian commented Nov 11, 2020

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chiragsalian commented Dec 8, 2020

Uh oh!

tgolen commented Dec 10, 2020

Uh oh!

chiragsalian commented Dec 10, 2020

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiragsalian commented Dec 10, 2020

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

chiragsalian commented Nov 5, 2020 •

edited

Loading