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

Add shortcut to maximize golden layout panes #3927

Merged
merged 6 commits into from
Mar 21, 2019
Merged

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Mar 20, 2019

This is the best I could come up with. keyboardjs doesn't easily allow to set a "target element" and keyup event handlers can only be attached at the document level, so I had to get a little more involved. What do you think? :)

Of course the shortcut is debatable, I picked M for Maximize, ideally it would be a single key press, but there have been voices in the community to avoid those.
The shortcut is . now.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open tracing view, move mouse over any pane and press . to toggle maximize.

Issues:


@daniel-wer daniel-wer self-assigned this Mar 20, 2019
@daniel-wer daniel-wer requested a review from philippotto March 20, 2019 19:33
@daniel-wer daniel-wer changed the title add ctrl + m shortcut to maximize golden layout panes Add ctrl + m shortcut to maximize golden layout panes Mar 20, 2019
@daniel-wer daniel-wer changed the title Add ctrl + m shortcut to maximize golden layout panes Add shortcut to maximize golden layout panes Mar 20, 2019
@fm3
Copy link
Member

fm3 commented Mar 20, 2019

just noting that ctrl+m is “mute tab” in firefox, but since wk doesn’t produce sound, I guess it is fair to overwrite that :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Sweet! With the shortcut, the maximize feature feels even better. However, I'm personally not a fan of ctrl+m, since I bound that to a different action in my browser :D However, that's my problem, I guess.

In general, a one-key short cut would be super cool, imo. Would enter be free? Wdyt @fm3 @normanrz regarding the shortcut? Even if invoked accidentally, the effect should be quite obvious to the user and the pane can even be unmaximized via the "maximize" button in the top right corner which should be intuitive.

keyboardjs doesn't easily allow to set a "target element" and keyup event handlers can only be attached at the document level, so I had to get a little more involved. What do you think? :)

Doesn't your code attach keyup at the document level? document.addEventListener("keyup", maximizeListener) doesn't contain a target, does it?

If I'm understanding it correctly an keyup handler is attached for each created component, right? So, for each key up event, approx. 10 listeners will fire. There is probably not a measurable difference, but I'm a bit hesitant with adding too many listeners, since we already have quite a lot of those 🙈
I played a bit around in the console and found the following way which would allow to only attach one listener (all hail the premature optimization!):

// ...
// attach one key up listener and if it's ctrl+m do the following
// ...
const hoveredComponents = glRoot.getItemsByFilter(item => {
  return item.isComponent && item.element[0].matches(":hover")
});
if (hoveredComponents.length > 0) {
  hoveredComponents[0].toggleMaximise();
}

What do you think?

@normanrz
Copy link
Member

I guess just m would be a good shortcut. I don't know what the concerns with single-key shortcuts are, but we do have a bunch of them already.

From a code perspective, would it be feasible to have a keyboard shortcuts defined in one place?

@daniel-wer
Copy link
Member Author

daniel-wer commented Mar 21, 2019

@philippotto Thanks for your feedback, you were right with both suggestions, I switched to using a single keyboardjs handler :)

The shortcut is now .

From a code perspective, would it be feasible to have a keyboard shortcuts defined in one place?

@normanrz I see the benefit of this, but I think it would be rather complicated or at least I don't have a good idea how to do that. We should definitely pay attention to keeping the shortcut overview up-to-date, so we're not losing track for now.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Very nice clean up 👍

@@ -104,13 +106,13 @@ export class GoldenLayoutAdapter extends React.PureComponent<Props<*>, *> {
if (onLayoutChange != null && this.gl.isInitialised) {
onLayoutChange(this.gl.toConfig(), this.props.activeLayoutName);
// Only when the maximized item changed, adjust css classes to not show hidden gl items.
if (this.maximizedItem !== !this.gl._maximisedItem) {
Copy link
Member

Choose a reason for hiding this comment

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

Wow, nice catch. Didn't spot in the other PR.

@normanrz
Copy link
Member

nice. looking forward to tweet about this 🐦

@daniel-wer daniel-wer merged commit 641094b into master Mar 21, 2019
@daniel-wer daniel-wer deleted the maximize-shortcut branch March 21, 2019 15:03
hotzenklotz added a commit that referenced this pull request Mar 25, 2019
…ture-highlight

* 'master' of github.com:scalableminds/webknossos:
  Hide unreported datasets (#3883)
  Update puppeteer and refresh screenshots (#3914)
  only show team names of own organization (#3928)
  Enable merger mode in skeleton and hybrid tracings (#3619)
  allow uploading nml for public dataset of different orga (#3929)
  Always make wheel listeners not passive to allow preventDefault (#3939)
  Enhance tree search functionallity (#3878)
  add webknossos-connect to setup (#3913)
  Update README.md (#3923)
  Add shortcut to maximize golden layout panes (#3927)
  Perform bucket picking in web workers and other performance optimizations (#3902)
  remove alt text for abstract brain loading image (#3930)
  updated documentation front page (#3917)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add shortcut to maximize pane
4 participants