Skip to content

Implement the focus_room_filter action#4560

Merged
lukebarnard1 merged 4 commits intodevelopfrom
luke/fix-ctrl-k
Jul 12, 2017
Merged

Implement the focus_room_filter action#4560
lukebarnard1 merged 4 commits intodevelopfrom
luke/fix-ctrl-k

Conversation

@lukebarnard1
Copy link
Copy Markdown
Contributor

@lukebarnard1 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 assigned lukebarnard1 and unassigned dbkr Jul 12, 2017
Comment thread src/components/structures/SearchBox.js Outdated
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';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that it needs a new then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/components/structures/SearchBox.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that would be simpler

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@lukebarnard1
Copy link
Copy Markdown
Contributor Author

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
@richvdh
Copy link
Copy Markdown
Member

richvdh commented Jul 13, 2017

Yarp fixed in #4564 I hope

@t3chguy t3chguy deleted the luke/fix-ctrl-k branch May 12, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants