Implement shift-click and ctrl-click semantics for TP #1641
Conversation
|
travis-ci/travis-ci#8836 is causing Chrome not to start. The tests pass locally so let's merge (pending review). |
| @@ -50,6 +50,8 @@ const TagTile = React.createClass({ | |||
| dis.dispatch({ | |||
| action: 'select_tag', | |||
| tag: this.props.groupProfile.groupId, | |||
| ctrlOrCmdKey: e.metaKey || e.ctrlKey, | |||
| shiftKey: e.shiftKey, | |||
There was a problem hiding this comment.
Hmm, so:
-
This will also catch the windows key on windows and the ctrl key on mac, which isn't quite what you want. Fortunately we've already written what you want in https://github.com/matrix-org/matrix-react-sdk/blob/master/src/components/structures/LoggedInView.js#L156 (which should totally be factored out... ;) )
-
I'm not really keen on passing the state of the modifier keys here: I'd have thought it should be up to the thing presenting the UI to decide what the modifier keys do.
| // the next range starts with an unselected tag. | ||
| if (!this._state.selectedTags.includes(payload.tag)) { | ||
| this._setState({ | ||
| anchorTag: payload.tag, |
There was a problem hiding this comment.
This isn't really state as you don't need to re-render as a result of this (I assume?)
|
And so a lengthy discussion was had between Dave and me about what should be keeping state and what sort of actions we should be sending about our flux part of the app. To summarise, we should only care if two views are using the one store. This simplifies the current implementation but makes the store highly coupled with the view. If we were to change that, we'd need to think long and hard about what would happen if two different views gave a way to interact with the store state, especially if they both display it because the user is interacting with that view based on the view, not what's kept in the store. The only practical case we could think of was a button somewhere (GroupView, possibly) which, when clicked, would select the group tag. This would absolutely not need the shift-click semantics that are currently buried in the store. In fact the shift-click semantics should only be known by the TagPanel itself. This would imply that the view would end up sending not "select_tag" or even "select_range_between_given_indices", but something closer to "select_these_tags_constructively" when holding ctrl and "select_these_tags_destructively" otherwise. This would mean sending chunks of "state" to the store, but arguably the store could only allow tags that it knows about, validating actions in a way. We agreed, however, that the added complexity was not needed for now. It was a fascinating discussion though... 🤓 |
as opposed to the incorrect ctrl || meta
src/Keyboard.js
Outdated
| @@ -58,3 +59,12 @@ module.exports = { | |||
| KEY_Y: 89, | |||
| KEY_Z: 90, | |||
| }; | |||
|
|
|||
| export function isCtrlOrCmdKeyEvent(ev) { | |||
There was a problem hiding this comment.
might be nice to call this isOnlyCtrlOrCmdKeyEvent or something to spell out that it's explicitly not going to match shift-cmd or meta-cmd etc.
|
Tests pass locally |
No description provided.