Style inbox differently when filter is non-empty #8130

akalin-keybase · 2017-08-16T04:53:08Z

Just show the participants' names, and move the filter-matching
names to the front.

Split Usernames component into PlaintextUsernames and Usernames.

Split off TopLine, BottomLine, and Avatars into their own files.

Split TopLine component into FilteredTopLine and SimpleTopLine.

Split Row into SimpleRow and FilteredRow.

akalin-keybase · 2017-08-16T04:53:38Z

r? @keybase/react-hackers

keybase-ci-visdiff · 2017-08-16T05:04:05Z

The commits 9385d8a...9f7c93c introduce visual changes on linux.

🔎 26 changed

Changed: Menubar-Public_Truncated (before) (after)
Changed: Menubar-Public_Normal (before) (after)
Changed: Menubar-Public_Menu_Showing (before) (after)
Changed: Menubar-Private_Truncated (before) (after)
Changed: Menubar-Private_Normal (before) (after)
Changed: Menubar-Private_Menu_Showing (before) (after)
Changed: Menubar-Private_Async_Writing (before) (after)
Changed: Menubar-Badge_People_Chat (before) (after) (diff)
Changed: Menubar-Badge_People (before) (after) (diff)
Changed: Menubar-Badge_Folder (before) (after) (diff)
Changed: Menubar-Badge_Device (before) (after) (diff)
Changed: Menubar-Badge_Chat (before) (after) (diff)
Changed: Folders_TLF-Normal_Public_Hide_Ignored (before) (after) (diff)
Changed: Folders_TLF-Normal_Public (before) (after) (diff)
Changed: Folders_TLF-Normal_Private_Hide_Ignored (before) (after) (diff)
Changed: Folders_TLF-Normal_Private (before) (after) (diff)
Changed: Chat_Inbox-You_Rekey_Selected (before) (after) (diff)
Changed: Chat_Inbox-You_Rekey (before) (after) (diff)
Changed: Chat_Inbox-Selected_Normal (before) (after) (diff)
Changed: Chat_Inbox-Selected_Muted (before) (after) (diff)
Changed: Chat_Inbox-Part_Rekey_Selected (before) (after) (diff)
Changed: Chat_Inbox-Part_Rekey (before) (after) (diff)
Changed: Chat_Inbox-Normal (before) (after) (diff)
Changed: Chat_Inbox-Long_Top (before) (after) (diff)
Changed: Chat_Inbox-Long_Bottom (before) (after) (diff)
Changed: Chat_Inbox-Empty (before) (after) (diff)

akalin-keybase · 2017-08-16T05:17:38Z

Hmm...not sure why this would cause visual changes. I'll investigate tomorrow.

akalin-keybase · 2017-08-18T18:13:32Z

Okay, rebased, ready for a look. I think the visual changes went away?

chrisnojima

I think it'll likely be better longer term if the filter concept isn't leaked to these components. like Usernames shouldn't know about a filter at all. if we're doing something like changing the sorting, we should change the sort before passing it to this component. We're attempting to make our components dumber and things like our connectors smarter.

I think likely the best approach is if you're in filter mode, we have a different list thats rendered entirely. It can render itself with some of the same sub components (like TopLine) but I don't think we want to keep the original list and plumb through more details the components inside need to know.
While the 1:1 convos will appear pretty close the team stuff will be entirely different, and longer term the list structure will be different so i think its likely best to divide it further upstream. lets discuss more on slack

chrisnojima · 2017-08-18T18:20:34Z

shared/common-adapters/usernames.js

+  return name.toLowerCase().indexOf(filter.toLowerCase()) >= 0
+}
+
+const sortNamesWithFilter = (names: Array<string>, filter: ?string): Array<string> => {


i don't think we change the sorting at all, see the design: https://zpl.io/aRMyk0V
search is 'malg' we don't change the tlf order though

Ahhh, I had thought that 'jack,cjb,miles,mark' in https://app.zeplin.io/project/56b25ffc64fc91773a73aaf8/screen/598a3926676fff2f2ab6f780 was 'jack,cjb,miles,mark,...,malg', and it turned into 'jack,cjb,miles,malg'. Will fix.

akalin-keybase · 2017-08-19T00:17:51Z

shared/chat/inbox/index.native.js

@@ -96,7 +97,7 @@ class Inbox extends PureComponent<void, Props, {rows: Array<any>}> {
          keyExtractor={this._keyExtractor}
          renderItem={this._renderItem}
          onViewableItemsChanged={this._onViewChanged}
-          getItemLayout={(data, index) => ({length: 64, offset: 64 * index, index})}
+          getItemLayout={(data, index) => ({index, length: 64, offset: 64 * index})}


eslint complained about key ordering here

this is actually a warning in eslint. we've not moved forward with making it an error. i'm kinda wishing it was something that could be auto fixed by lint. i tend to sort it as i think it makes things cleaner but its def. not required just fyi

akalin-keybase · 2017-08-19T00:20:19Z

shared/chat/inbox/row/avatars.js

+import memoize from 'lodash/memoize'
+import {List} from 'immutable'
+
+// All this complexity isn't great but the current implementation of avatar forces us to juggle all these colors and


Didn't change much here, except made the components functional

Reverted making it functional

akalin-keybase · 2017-08-19T00:41:02Z

shared/chat/inbox/row/bottom-line.js

+  youNeedToRekey: boolean,
+}
+
+function BottomLine(props: BottomLineProps) {


Didn't change much here, except made the components functional

actually we should keep these as PureComponent as they're optimized to not rerender w/ shallowcompare. react hasn't yet made a pass at optimizing this sadly

Ah, okay. I assume you mean this and all the others I converted? Will do...

Converted everything back. Can you explain a bit more about why functional components aren't as optimized in this case? Is it just a general thing that inheriting from PureComponent does the right thing whereas functional components don't?

Purecomponent has a shallow equal compare of props and state so if those === it'll not rerender. Currently functions don't do this check (even though it's safe for us). We could wrap with recompose and it has a pure helper or we can just extend from pure component.

facebook/react#5677

akalin-keybase · 2017-08-19T00:41:46Z

shared/chat/inbox/row/simple-row.js

-              type="BodySemibold"
-              plainDivider={isMobile ? undefined : ',\u200a'}
-              containerStyle={{...boldOverride, color: usernameColor, paddingRight: 7}}
-              users={teamname ? [{username: teamname}] : participants.map(p => ({username: p})).toArray()}


we don't need this teamname logic anymore, right, since we branched those off into their own components?

i'm bringing this back. the row for this and teams that are simple teams is so close its not worth splitting it up. the thing thats key to be different is for the filtered stuff

akalin-keybase · 2017-08-19T00:42:38Z

shared/chat/inbox/row/top-line.js

+  const {participants, showBold, usernameColor} = props
+  const boldOverride = showBold ? globalStyles.fontBold : null
+  return (
+    <Box


Collapsed the two outer Boxes into one here

akalin-keybase · 2017-08-19T00:43:03Z

shared/common-adapters/plaintext-usernames.js

+
+import type {Props} from './plaintext-usernames'
+
+const inlineStyle = isMobile


There's some duplication here with usernames.js...should I try and factor those out?

why split this out at all? its used by filtered and non filtered right

Done, moved back into usernames.js

akalin-keybase · 2017-08-19T00:43:55Z

All right, I took a crack at moving the logic further up and using branch. This PR got a lot bigger, though, so let me know if I should split it up into smaller ones. PTAL!

akalin-keybase · 2017-08-19T04:05:38Z

shared/chat/inbox/row/container.js

@@ -146,7 +146,8 @@ const ConnectedRow = compose(
  // $FlowIssue
  pausableConnect(mapStateToProps, mapDispatchToProps, mergeProps),
  branch(props => props.teamname && !props.channelname, renderComponent(TeamRow)),
-  branch(props => props.teamname && props.channelname, renderComponent(ChannelRow))
+  branch(props => props.teamname && props.channelname, renderComponent(ChannelRow)),
+  branch(props => props.filtered, renderComponent(FilteredRow))


I noticed that flow doesn't catch, e.g. mistakenly putting props.filter or something -- will this be fixed once we update to 0.53? Is that what the $FlowIssue tag above is?

Hopefully. There's a flow-types recompose that'll be out but it's not there yet

akalin-keybase · 2017-08-19T04:07:46Z

Addressed latest comments, PTAL!

akalin-keybase changed the base branch from master to akalin/DESKTOP-4391-inbox-filter-ui August 16, 2017 04:53

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-ui branch from e70d369 to 4789f10 Compare August 16, 2017 18:53

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-style branch 2 times, most recently from 6f87abd to 32fc188 Compare August 16, 2017 18:56

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-ui branch from 4789f10 to 5ebd64d Compare August 18, 2017 00:02

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-style branch from 32fc188 to e96d582 Compare August 18, 2017 05:14

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-ui branch from 5ebd64d to 224cf55 Compare August 18, 2017 17:02

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-style branch 2 times, most recently from 1cc12fd to f5bb6bc Compare August 18, 2017 18:12

akalin-keybase changed the base branch from akalin/DESKTOP-4391-inbox-filter-ui to master August 18, 2017 18:12

chrisnojima requested changes Aug 18, 2017

View reviewed changes

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-style branch from f5bb6bc to 37cd242 Compare August 18, 2017 21:57

akalin-keybase commented Aug 19, 2017

View reviewed changes

chrisnojima approved these changes Aug 21, 2017

View reviewed changes

akalin-keybase added 4 commits August 22, 2017 01:59

Switch based on filter

29da0c0

Change topline depending on filter

1cdd063

Add rudimentary filtering

96439b4

Centralize filter logic

399ca45

akalin added 27 commits August 22, 2017 01:59

Fix compile

a35de8e

Revert sorting

999ce9d

Don't pass down filter to usernames

23811b5

Move TopLine into its own file

97e6262

Move BottomLine to its own file

69b9144

Move Avatars and TeamAvatar into their own file

db2734d

Pass down filtered instead of filter

9d4b4b7

Branch into simple and filtered row

9a3528a

Always use avatars

417c888

More cleanup

f042aae

Rem more props

4d6e9c0

Rem more

4ef6830

Collapse boxes

f9bc1b4

Pull out into PlaintextUsernames

c9ec7ec

Remove unneeded

d30c1cb

Rem style

4c86da0

More cleanup

1d51d5d

Use functional

22a4d5f

Clean up BottomLine

71560df

Fixup Avatars

5fe6d7e

Fixup row classes

66d371c

Fix lint

725d06e

Merge PlaintextUsername and Username

944ba3b

Change TopLine back to non-functional

3d86cc7

Change BottomLine back to non-functional

2500b0b

Revert Avatars

f38167b

Revert SimpleRow et al

9c96e05

akalin-keybase force-pushed the akalin/DESKTOP-4391-inbox-filter-style branch from 987ac29 to 9c96e05 Compare August 22, 2017 09:00

akalin-keybase merged commit d3270ec into master Aug 22, 2017

akalin-keybase deleted the akalin/DESKTOP-4391-inbox-filter-style branch August 22, 2017 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style inbox differently when filter is non-empty #8130

Style inbox differently when filter is non-empty #8130

akalin-keybase commented Aug 16, 2017 •

edited

Loading

akalin-keybase commented Aug 16, 2017

keybase-ci-visdiff commented Aug 16, 2017

akalin-keybase commented Aug 16, 2017

akalin-keybase commented Aug 18, 2017

chrisnojima left a comment

chrisnojima Aug 18, 2017

akalin-keybase Aug 18, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 19, 2017

akalin-keybase Aug 19, 2017 •

edited

Loading

akalin-keybase Aug 19, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 19, 2017

akalin-keybase Aug 19, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 19, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 22, 2017

akalin-keybase Aug 19, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 19, 2017

akalin-keybase Aug 19, 2017

akalin-keybase commented Aug 19, 2017

akalin-keybase Aug 19, 2017

chrisnojima Aug 19, 2017

akalin-keybase commented Aug 19, 2017


		import type {Props} from './plaintext-usernames'

		const inlineStyle = isMobile

Style inbox differently when filter is non-empty #8130

Style inbox differently when filter is non-empty #8130

Conversation

akalin-keybase commented Aug 16, 2017 • edited Loading

akalin-keybase commented Aug 16, 2017

keybase-ci-visdiff commented Aug 16, 2017

akalin-keybase commented Aug 16, 2017

akalin-keybase commented Aug 18, 2017

chrisnojima 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

akalin-keybase Aug 19, 2017 • 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

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

akalin-keybase commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalin-keybase commented Aug 19, 2017

akalin-keybase commented Aug 16, 2017 •

edited

Loading

akalin-keybase Aug 19, 2017 •

edited

Loading