Add a chat switcher to the LHN #282

tgolen · 2020-08-21T21:14:50Z

Fixes #269

Refactors the sidebar component to break it up into smaller and more focused pieces
Adds a chat switcher
Adds a Command+K keyboard shortcut to access the chat switcher

Tests

Press Command+K and verify the chat switcher gets focused
Begin typing names or emails and see the list of people appear
Use the arrow keys to highlight different options
Press the enter key to select an option (or click on an option)

Things left to do

style it (@shawnborton )
hook up a method to get the reportID and redirect the user to that report

marcaaron

Looks good so far! Just left some initial comments mostly around the ChatSwitcher which I'm guessing has some copy/paste stuff going on

marcaaron · 2020-08-21T22:12:12Z

src/page/home/sidebar/ChatSwitcherView.js

+
+    // All of the personal details for everyone
+    // Eslint is disabled here because personalDetails is an object where they keys are email addresses
+    // so it can't be documented in proptypes


that's what PropTypes.objectOf() is for

marcaaron · 2020-08-21T22:24:35Z

src/page/home/sidebar/ChatSwitcherView.js

+    constructor(props) {
+        super(props);
+
+        this.maxSearchResults = 10;


This is a constant can we move it outside the component?

marcaaron · 2020-08-21T22:36:09Z

src/page/home/sidebar/ChatSwitcherView.js

+     * @param {number} oldFocusedIndex
+     * @param {number} newFocusedIndex
+     */
+    switchFocusedIndex(oldFocusedIndex, newFocusedIndex) {


Alternatively, we could track the focused index in state and re-render the options. Seems simpler to do that vs. having an option with a focused property.

e.g. if index === this.state.focusedIndex do whatever we need to do on render or pass focused as a prop.

Ah yes, the web-e component puts this in the state too, and I actually felt it was better to leave it out of the state. If you look at that component, there is a bunch of stuff in state, and I was trying to clean that up. However, I think putting it into state will be fine and I'd like to keep it closely aligned with web-e.

marcaaron · 2020-08-21T23:20:43Z

src/page/home/sidebar/ChatSwitcherView.js

+    }
+
+    /**
+     * Every time the text changes in the TextInput, update the search value in the state


This does a lot more than this

marcaaron · 2020-08-21T23:21:25Z

src/page/home/sidebar/ChatSwitcherView.js

+     * @param {string} value
+     */
+    updateSearch(value) {
+        this.setState({search: value});


Can this setState call be combined with the one below?

src/page/home/sidebar/ChatSwitcherView.js

marcaaron · 2020-08-21T23:30:17Z

src/page/home/sidebar/ChatSwitcherView.js

+            new RegExp(Str.escapeForRegExp(value), 'i'),
+        ];
+
+        // Use a Set so that duplicate values are automatically removed


Pretty sure this statement is false if your Set values are objects. You could use a Map and use something unique like the login as the key.

Hmm, you're right. I thought this cleared up a bug, but maybe it was something else.

Actually, I found out that the Set is still good to use here. Looking at the MDN docs, the equality check also works to remove the same instances of an object. Example:

The same obj1 is added to the set twice and it is not duplicated.

Ok I was presuming that we would not want to add two objects that had the same shape and values. But maybe that is wrong?

marcaaron · 2020-08-21T23:31:39Z

src/page/home/sidebar/ChatSwitcherView.js

+
+        // Set the first option to be focused
+        if (options.length) {
+            options[0].focused = true;


More extra logic needed to accommodate the focused property

marcaaron · 2020-08-21T23:34:44Z

src/page/home/sidebar/ChatSwitcherView.js

+                        ref={el => this.textInput = el}
+                        style={[styles.textInput, styles.textInputReversed, styles.flex1]}
+                        value={this.state.search}
+                        onBlur={() => this.state.search === '' && this.props.onBlur()}


This is interesting. But I think might look cleaner with brackets and a if for early return

src/page/home/sidebar/ChatSwitcherView.js

roryabraham

High-level, I have three main comments here:

It's not compatible with desktop, but fixing those issues to get React Native to play nice with Electron might be challenging, and probably warrants its own issue/PR. That being said, I do think it's doable: there's an early-stage npm project addressing some of the challenging pieces, such as hotkeys, links, dialogs/alerts, and clipboard. While it doesn't offer stability yet, it's open-source I'm sure the developer would welcome PR's. If we don't like that idea, maybe we could also just work-around it by having separate files when we need to use some Electron-specific code for desktop (i.e: index.desktop.js)
Beyond the actual functionality, the download links need to detect the current platform to show the other three.
There were a handful of instances where you used underscore functions for which there is a native-javascript equivalent, and I think it might be best to stick with plain JS whenever convenient. In some cases I think it's more performant too, i.e: not extending an object with all the underscore methods like _(myObject), just to use one function.

src/lib/KeyboardShortcut/index.js

roryabraham · 2020-08-24T00:59:53Z

src/lib/KeyboardShortcut/index.js

+    let cb = events[event.keyCode];
+    cb = cb[cb.length - 1];
+
+    const pressedModifiers = _.all(cb.modifiers, (modifier) => {


NAB: AFAIK we don't have style guide recommendations on this matter, but I would prefer the pure-js alternative to underscore:

cb.modifiers.every((modifier) => { ... });

roryabraham · 2020-08-24T01:21:33Z

src/lib/KeyboardShortcut/index.js

+    }
+
+    // The active callback is the last element in the array
+    let cb = events[event.keyCode];


Prefer const:

const cb = _.last(events[event.keyCode]);

src/page/home/sidebar/AppLinks.js

src/page/home/sidebar/ChatSwitcherView.js

roryabraham · 2020-08-24T02:26:15Z

src/page/home/sidebar/SidebarBottom.js

+
+const propTypes = {
+    // Safe area insets required for mobile devices margins
+    // eslint-disable-next-line react/forbid-prop-types


I think you could do this (see here):

insets: PropTypes.objectOf(PropTypes.shape( top: PropTypes.number, left: PropTypes.number, right: PropTypes.number, bottom: PropTypes.number, )),

But this prop is drilled down through several layers, so another alternative that I've used in the past is saving some special or complex prop types that are reused multiple times into a file/module of their own (usually just a directory called types and export a PropType object from each file). Not sure whether that's a good choice here.

src/page/home/sidebar/SidebarLinks.js

quinthar · 2020-08-24T06:10:53Z

I think you should include the Android link at least, because on a desktop web browser if you click through to an Android app, you can trigger it to be installed onto your Android phone even from a desktop browser.

…

On Sun, Aug 23, 2020, 9:46 PM Rory Abraham ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/page/home/sidebar/AppLinks.js <#282 (comment)> : > +import Anchor from '../../../components/Anchor'; + +const AppLinks = () => { + // Give instructions for downloading the desktop app + function alertInstallInstructions() { + alert('To install Chat Desktop App:\n\n' + + '1. Double click "Chat.app.zip"\n' + + '2. If you get a "Cannot be opened because the developer cannot be verified" error:\n' + + ' 1. Hit Cancel\n' + + ' 2. Go to System Preferences > Security & Privacy > General\n' + + ' 3. Click Open Anyway at the bottom\n' + + ' 4. When it re-prompts you, click Open'); + Linking.openURL('https://chat.expensify.com/Chat.app.zip'); + } + + return ( Side note: does it make much sense for a desktop app to include links to iOS/Android? Probably not. I think we should only show this on web, noop it for desktop (we're already nooping it for iOS/Android) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#282 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUVQZAVC4YWPBEFAEZ3SCHWATANCNFSM4QHVQD5A> .

tgolen · 2020-08-26T20:03:18Z

Updated with conflic fixes and improvements from Marc's review. The StyleSheet.flatten() and StyleSheet.compose() are great!

roryabraham

Looks great, just the one PropTypes concern!

roryabraham · 2020-08-26T20:30:16Z

src/components/TextInputWithFocusStyles.js

+
+    // Styles to apply to the text input when it has focus
+    // eslint-disable-next-line react/forbid-prop-types
+    styleFocusIn: PropTypes.any,


I think you can:

import {ViewPropTypes} from 'react-native';

and then:

styleFocusIn: PropTypes.arrayOf(ViewPropTypes.style),

Hmm, I tried that, and ViewPropTypes is undefined... Any idea why?

Maybe because it's deprecated and not exported from react-native-web: reactrondev/react-native-web-swiper#41

hmmm... maybe PropTypes.instanceOf(StyleSheet) ?

NAB if you can't find a good way to typecheck this, but I'd be shocked if there wasn't

roryabraham

oops, meant to request the small typechecking change

roryabraham

Changing to NAB

marcaaron · 2020-08-27T22:38:20Z

LGTM and doesn't seem to be any blockers now so I am merging.

Bump JS-Libs version for PR #282

tgolen added 13 commits August 21, 2020 09:52

Rename directories and move files around a little

fa0e2e5

Refactor the sidebar into separate components

a45f4e5

Rename file

72b4bb9

Add initial search form

30a715d

Hide and show the report list on blur and focus

ba49562

Remove color

f5d8207

Remove extra method

804b5c5

Get the search options to appear

1d04aad

Reset the view on tab and escape

989a4c2

Show focused option

94e9d7d

Select the option and reset the component

3578e59

Add keyboard shortcut lib

3ced899

Enable the command+k keyboard shortcut

c15b31e

tgolen requested review from marcaaron and roryabraham August 21, 2020 21:14

tgolen self-assigned this Aug 21, 2020

tgolen added 2 commits August 21, 2020 16:20

Make the clear button do a full reset

5e33acc

Add some comments and shorten line

a51e81b

marcaaron reviewed Aug 21, 2020

View reviewed changes

Add some styles and the avatar to the list

358a970

roryabraham requested changes Aug 24, 2020

View reviewed changes

tgolen added 8 commits August 24, 2020 12:55

Merge branch 'master' into tgolen-chat-switcher

3527824

Fix paths

7cc9ebb

Move focued index into the state

5c6ef87

Make logic more explicit

aa5ff2e

DRY up proptypes for insets

972832e

Add more comments about using Set

632b596

More comment cleanup

fbe8937

Enable key listening when typing a comment

21116f8

tgolen added 7 commits August 26, 2020 13:44

Simplify keyboard shortcut

5a9cda1

Use a state property for if the component is focused

2f388d5

Use a variable for focused input

6e052d7

Improve style merging

3dbd102

Merge branch 'master' into tgolen-chat-switcher

f259284

Fix casing for HOC

28d7e02

Lint

f7bba83

roryabraham approved these changes Aug 26, 2020

View reviewed changes

roryabraham requested changes Aug 26, 2020

View reviewed changes

roryabraham previously approved these changes Aug 26, 2020

View reviewed changes

tgolen added 3 commits August 27, 2020 10:31

Add a method for getting a chat report

b950723

Implement fetching the chat report and redirecting to it

e9ac597

Put in the proper API params

9aa1820

tgolen dismissed roryabraham’s stale review via 9aa1820 August 27, 2020 16:40

tgolen added 10 commits August 27, 2020 10:42

Improve comments and fix lint

075ed9d

Some last minute testing fixes

a6efbfe

Merge branch 'master' into tgolen-chat-switcher

90e9c80

Fix some lint errors

a359f1b

Get the report name for the new report

6ec2ffa

Remove redundant request

26c8b5d

Always merge personal details

a8732f8

Remove extra API call

9b3d307

lint

5fbde0a

Fix report name

6ca354b

marcaaron approved these changes Aug 27, 2020

View reviewed changes

marcaaron merged commit 7d04aed into master Aug 27, 2020

marcaaron deleted the tgolen-chat-switcher branch August 27, 2020 22:38

sketchydroide added a commit that referenced this pull request Oct 16, 2020

Merge pull request #659 from Expensify/maria-bump-js-libs-version

82fe648

Bump JS-Libs version for PR #282

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a chat switcher to the LHN #282

Add a chat switcher to the LHN #282

tgolen commented Aug 21, 2020

marcaaron left a comment

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

tgolen Aug 24, 2020

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

tgolen Aug 24, 2020

tgolen Aug 24, 2020

marcaaron Aug 24, 2020

marcaaron Aug 21, 2020

marcaaron Aug 21, 2020

roryabraham left a comment

roryabraham Aug 24, 2020

roryabraham Aug 24, 2020

roryabraham Aug 24, 2020

tgolen Aug 24, 2020

quinthar commented Aug 24, 2020 via email

tgolen commented Aug 26, 2020

roryabraham left a comment •

edited

Loading

roryabraham Aug 26, 2020

tgolen Aug 26, 2020

tgolen Aug 26, 2020

roryabraham Aug 26, 2020

roryabraham Aug 26, 2020

roryabraham left a comment

roryabraham left a comment

marcaaron commented Aug 27, 2020

Add a chat switcher to the LHN #282

Add a chat switcher to the LHN #282

Conversation

tgolen commented Aug 21, 2020

Tests

Things left to do

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinthar commented Aug 24, 2020 via email

tgolen commented Aug 26, 2020

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

marcaaron commented Aug 27, 2020

roryabraham left a comment •

edited

Loading