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

[Canvas] Feat: Keyboard shortcuts for nudging elements #39208

Merged
merged 6 commits into from
Jun 25, 2019

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Jun 18, 2019

Summary

Related to #23058.

Jun-18-2019 13-00-01

This PR adds shortcuts to shift elements up, down, left, and right by intervals of 16px or 1px.

Keyboard Shortcuts added:
Screen Shot 2019-06-18 at 1 04 05 PM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@ryankeairns
Copy link
Contributor

I wonder if we need all the shortcut permutations... could we make it be just one for each? Like 'Nudge by 1px' then the shortcut shows the 4 different arrow keys all on one line? or generically say [arrow keys]

@cqliu1
Copy link
Contributor Author

cqliu1 commented Jun 18, 2019

@ryankeairns I have to map them all separately to perform different actions in public/lib/keymap which is what the shortcut reference uses to generate the content. I don't think there's a way we can combine them into one line

@cqliu1 cqliu1 force-pushed the feat/nudge-shortcuts branch from 7d2ca27 to 6ae2f94 Compare June 18, 2019 20:46
@cqliu1 cqliu1 requested a review from monfera June 18, 2019 20:57
@cqliu1 cqliu1 added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Jun 18, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@cqliu1 cqliu1 changed the title [Canvas] Adds keyboard shortcuts for nudging elements [Canvas] Feat: Adds keyboard shortcuts for nudging elements Jun 18, 2019
@cqliu1 cqliu1 added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort review v7.3.0 v8.0.0 labels Jun 18, 2019
@cqliu1 cqliu1 requested a review from ryankeairns June 18, 2019 20:58
@cqliu1 cqliu1 marked this pull request as ready for review June 18, 2019 20:59
@cqliu1 cqliu1 requested a review from a team as a code owner June 18, 2019 20:59
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cqliu1 cqliu1 changed the title [Canvas] Feat: Adds keyboard shortcuts for nudging elements [Canvas] Feat: Keyboard shortcuts for nudging elements Jun 18, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ryankeairns
Copy link
Contributor

@cqliu1 ok.

Last question, it's actually 10px not 16px on the big nudge, right? I think 10px, in this case, is the best/expected result.

@cqliu1
Copy link
Contributor Author

cqliu1 commented Jun 18, 2019

Yep, I was playing around with different numbers, but I can change it back to 10px.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@monfera
Copy link
Contributor

monfera commented Jun 19, 2019

Sorry, I suggested 16px on vague recollection that design tools and grid systems use power of 2 increments, and this way, it's faster to cover a larger distance (accessibility), while it still takes a maximum of 8 small (1px) nudges to fine tune (8 px back or forth if the user ends up in the middle of the larger nudge). I just checked Google Sheets, it looks like it also uses around 10px.

The functionality is working well for individual nudges, and the element stops moving if I keep pressing the arrow, and it'll eventually teleport to the new location (on my backup machine that's 2014 vintage and my Key Repeatin System Preferences is set to highest). It's likely because every single increment is persisted now in Redux, otherwise, with the current approach, the element wouldn't move (ie. we can't just debounce). On every Redux access it'll also persist the element in the Elasticsearch index. While that could be debounced, it'd probably make asynchronous randomness worse, and it may not even be responsible for the lag.

So we seem to have these options:

  1. On the assumption that people nudge with only a few keystrokes, it's good as it is (in this case, it's not too accessible for cover larger distances, though we lack accessible keyboard controls at the moment, eg. resize, rotate, selections etc.)
  2. It's done through the layout engine, which, for the mouse drag now, only issues a Redux repositioning action only upon the end of the gesture (which in this case could be the keyup event)

As the linked issue expressly mentions accessibility, I feel like a keyboard-only user should be able to move an element at larger distances, ie. doing it via the deferred route with the layout engine, and perhaps using larger step sizes or adding another modifier key that induces larger jumps eg. 50-64px.

But I'm not sure if this improvement is needed in this specific PR or it should be a follow-up PR.

Any thoughts on this overall? Should we loop in MikeB as I'm second-guessing accessibility aspects.

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks and works great for me.

I don't see any staggering like Robert mentioned, but definitely could see that being an issue with the constant update cycles happening. And having a persist happen on every press could also be an issue. Might be worth spending some time to think of some way to prevent that from happening.

Might also be a nice feature to have elements snap to edges as you nudge them that way, but that's definitely for another PR

@cqliu1 cqliu1 force-pushed the feat/nudge-shortcuts branch from b275310 to 4cc42c2 Compare June 20, 2019 17:43
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

👍 LGTM. I've longed for this feature and can't wait to use it! The functionality works as described and is a great addition as-is. +1 to handling any rough edges in a follow-up PR.

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Great work, in agreement with those who spake before me!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cqliu1 cqliu1 merged commit fd32269 into elastic:master Jun 25, 2019
@cqliu1 cqliu1 deleted the feat/nudge-shortcuts branch June 25, 2019 16:29
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jun 25, 2019
* Added nudge shortcuts

* Cleaned up props

* Updated storyshot

* Changed offset back to 10

* Updated keyboard shortcuts storyshot
cqliu1 added a commit that referenced this pull request Jun 25, 2019
* Added nudge shortcuts

* Cleaned up props

* Updated storyshot

* Changed offset back to 10

* Updated keyboard shortcuts storyshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:enhancement review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants