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

Implement the focus_room_filter action #4560

Merged
merged 4 commits into from Jul 12, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Jul 12, 2017

This is for ctrl+k room filtering and switching

codep matrix-org/matrix-react-sdk#1212

This is for ctrl+k room filtering and switching
@dbkr
dbkr approved these changes Jul 12, 2017
@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Jul 12, 2017
Which was a74bbb4
import KeyCode from 'matrix-react-sdk/lib/KeyCode';
import sdk from 'matrix-react-sdk';
import dis from 'matrix-react-sdk/lib/dispatcher';
import RateLimitedFunc from 'matrix-react-sdk/lib/ratelimitedfunc';

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 12, 2017

Member

I would keep the style of this import if possible: it's a function rather than a class / component.

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jul 12, 2017

Author Contributor

Weird that it needs a new then?

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 12, 2017

Member

Basically the new is to get the this correct in the function, but it's still creating a function rather than an instance of a class, notionally.

@@ -90,6 +90,17 @@ module.exports = React.createClass({
}
},

_onKeyDown: function(ev) {
// Only do anything when the key event target is the search input
if(ev.target !== this.refs.search) return;

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 12, 2017

Member

Why not just listen for keydown events on the appropriate element?

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jul 12, 2017

Author Contributor

Yep, that would be simpler

dis.dispatch({
action: 'view_room',
room_id: roomId,
clear_search: (ev && (ev.keyCode == KeyCode.ENTER || ev.keyCode == KeyCode.SPACE)),

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 12, 2017

Member

I'm a bit baffled here: why would a click event have a keyCode?

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Jul 12, 2017

Author Contributor

This is for when you "clicked" it by focusing and hitting enter or space (which is what the previous impl did).

This comment has been minimized.

Copy link
@dbkr

dbkr Jul 12, 2017

Member

Oh, I think I see what's going on - so we rely on you tabbing to the RoomTile and hitting enter rather than the search box handling the enter, in which case this makes sense.

@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Jul 12, 2017
@dbkr
dbkr approved these changes Jul 12, 2017
@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Jul 12, 2017

This looks like a flaky, unrelated test failure
@richvdh ^ - https://travis-ci.org/vector-im/riot-web/jobs/252898233

@lukebarnard1 lukebarnard1 merged commit 248944a into develop Jul 12, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jul 13, 2017

Yarp fixed in #4564 I hope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.