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.

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

Optimise RoomList and implement quick-search functionality on it. #3654

Merged
merged 15 commits into from Apr 20, 2017

Conversation

@ara4n
Copy link
Member

ara4n commented Apr 18, 2017

This is effectively two PRs which ended up getting very entangled:

  1. Implements quick-search in SearchBox (listening for Ctrl/Cmd-K), and arrow-key based navigation through the resulting room list in LeftPanel.
  2. Splits a RoomSubListHeader out of RoomSubList, and switch to re-sorting/refreshing the list only on demand via pokes from RoomList using ConstantTimeDispatcher, rather than constantly having RoomList constantly replace the whole list property.

The latter is slightly controversial, as it deliberately short-circuits React from working out when to re-render the list, given we have no suitable properties to use (short of constantly generating new lists). It also technically doesn't need ConstantTimeDispatcher; it could also be done by calling the method directly on a reference to the right sublist, or even using the existing dispatchers.

// this.focusDirection = up;

var descending = false; // are we currently descending or ascending through the DOM tree?
var classes;

This comment has been minimized.

Copy link
@Kegsay

Kegsay Apr 19, 2017

Contributor

We should probably be writing all new code using let.

Copy link
Member

dbkr left a comment

Couple of minor points, other than that, fine, main high level comments made over in matrix-org/matrix-react-sdk#807

@@ -119,7 +136,7 @@ var RoomSubList = React.createClass({
// The header is collapsable if it is hidden or not stuck
// The dataset elements are added in the RoomList _initAndPositionStickyHeaders method
isCollapsableOnClick: function() {
var stuck = this.refs.header.dataset.stuck;
var stuck = this.refs.header.refs.header.dataset.stuck;

This comment has been minimized.

Copy link
@dbkr

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 19, 2017

Author Member

i agree it's ugly, but i don't think there's an alternative having moved the header into the sublist? i don't think there's NPE risk.

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

Yeah, the alternative would be a function ref, in which case you could probably pass the function through and then the ref goes straight into the function, but this isn't making things a whole lot worse, so meh.

} while(element && !(
classes.contains("mx_RoomTile") ||
classes.contains("mx_SearchBox_search") ||
classes.contains("mx_RoomSubList_ellipsis")));

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

I'm surprised you need to be relying on the CSS classes rather than being able to use the tabIndex somehow.

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 19, 2017

Author Member

the up/down keys deliberately ignore tab index but implement their own rules for navigating the list (e.g. skipping over the sublist headers, which you can still get at if you really want by tab/shift-tab)

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

Ah, I see.

var FormattingUtils = require('matrix-react-sdk/lib/utils/FormattingUtils');
var RoomNotifs = require('matrix-react-sdk/lib/RoomNotifs');
var AccessibleButton = require('matrix-react-sdk/lib/components/views/elements/AccessibleButton');
var ConstantTimeDispatcher = require('matrix-react-sdk/lib/ConstantTimeDispatcher');

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

Ftr, if you're writing new code, import rather than require please, and no use strict (although I guess this was c+ped).

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 19, 2017

Author Member

i've been trying to keep style consistent within a file, so we can then go and blitz files en masse (perhaps using one of the algorithmic JS rewriters). i really don't like having thoroughly inconsistent styles within the same file, as it just gets in the way of the code.

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

Yep, likewise - hence suggesting that if you're making a new file it should probably be in the new style rather than the old, but presumably this was taken out from an existing file rather than written from scratch.


componentWillMount: function() {
// constantTimeDispatcher.register("RoomSubList.refreshHeader", this.props.tagName, this.onRefresh);
},

This comment has been minimized.

Copy link
@dbkr

dbkr Apr 19, 2017

Member

Should probably just get rid of these if they aren't necessary

This comment has been minimized.

Copy link
@ara4n

ara4n Apr 19, 2017

Author Member

i left them in just in case we move the refreshHeader back to the child in the end - it got moved back to RoomSubList because we also have header info in the truncation ellipsis cells. agreed it's bad to leave commented out stuff hanging around though.

This comment has been minimized.

Copy link
@dbkr
@dbkr dbkr assigned ara4n and unassigned dbkr Apr 19, 2017
@ara4n

This comment has been minimized.

Copy link
Member Author

ara4n commented Apr 19, 2017

@dbkr PTAL at feedback

@ara4n ara4n removed their assignment Apr 19, 2017
@dbkr
dbkr approved these changes Apr 19, 2017
@dbkr

This comment has been minimized.

Copy link
Member

dbkr commented Apr 19, 2017

lgtm then, will leave you to merge, assuming the test fails are because of the co-dependent changes.

ara4n added 3 commits Apr 19, 2017
@ara4n ara4n merged commit 8f20fcf into develop Apr 20, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@lampholder lampholder added the notready label May 15, 2017
dbkr added a commit that referenced this pull request May 17, 2017
This reverts commit 8f20fcf, reversing
changes made to 751f715.
dbkr added a commit that referenced this pull request May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.