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 keyboard shortcut to temporarily hide all labels in the App #1779

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

ritch
Copy link
Contributor

@ritch ritch commented May 25, 2022

See background in #1761.

This feature adds the ability to temporarily hide all overlays/tooltips by pressing the "Shift" key. This is supported while viewing samples in the Looker modal image and video modes.

Specifically this adds:

  • a new control in the Looker
  • a corresponding button in the Looker controls
  • support for ControlEventKeyTypes which allow for specifying which style event your hotkey supports (HOLD = press and hold, KEY_DOWN/DEFALUT = current behavior).
  • a new Atom for persisting the show/hide overlay value in recoil
  • glue between the Looker and recoil (based on fullscreen)

To implement a control with "hold" supprt, you must specify it's ControlEventKeyTypes as HOLD as well as add a new method: control.afterAction(). This afterAction method will be called on keyup allowing for the control to make updates after the user depresses the key.

NOTE: in a typical hold event model I would usually debounce the keydown event. I've left that optimization out, if we notice performance issues we can look into adding that or something similar.

requirements/common.txt Outdated Show resolved Hide resolved
@ritch ritch requested a review from benjaminpkane May 25, 2022 14:45
@ritch ritch force-pushed the feat-1761 branch 4 times, most recently from 1315230 to c01def1 Compare May 25, 2022 20:36
@ritch ritch requested a review from benjaminpkane May 26, 2022 22:05
@ritch ritch changed the title WIP Feat 1761 Add keyboard shortcut to temporarily hide all labels in the App May 26, 2022
@ritch ritch marked this pull request as ready for review May 26, 2022 22:21
@ritch ritch self-assigned this May 31, 2022
@ritch ritch added feature Work on a feature request app Issues related to App features javascript Pull requests that update Javascript code labels May 31, 2022
Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

LGTM! Looks like HOLD was straightforward to add. Clean solution.

I wouldn't worry about debouncing. Updates are supposed to run in a single animation frame regardless.

this.element.src = ICONS.overlaysHidden;
} else {
this.element.title = `Show all overlays (h)`;
this.element.classList.add(lookerControlActive);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe reverse the icons? It looks like the strike through icon is shown when labels are visible. I would have thought that means they are hidden.

@brimoor
Copy link
Contributor

brimoor commented Jun 2, 2022

Couple things I saw while testing:

  • Tooltip still shows h as hotkey, should be shift
  • Agree with Ben that, to me, the icons should be swapped: strikethrough should be shown when the labels are hidden, not when they're visible
  • If I click on the new icon, then drag somewhere else (eg up onto canvas), then release, the show labels event is not triggered. I'm now in a state where I'm not holding shift or clicking the icon, but the labels are still hidden. Ideally this couldn't happen

@ritch
Copy link
Contributor Author

ritch commented Jun 2, 2022

Fixed the issues mentioned above. This should be ready to go now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Issues related to App features feature Work on a feature request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants