Add ability to expand or collapse all the children of selected node #621
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
frontend/keyboardNav.js
Outdated
| @@ -26,6 +26,9 @@ var keyCodes = { | |||
| '38': 'up', | |||
| '39': 'right', | |||
| '40': 'down', | |||
|
|
|||
| '69': 'expand', | |||
There was a problem hiding this comment.
Please change this to match Chrome shortcuts.
There was a problem hiding this comment.
I will have to add the shortcut to manifest.json of shells/chrome. Right now there is no support for shortcut in any of the plugins. Will spend some time to try to figure out how to do this.
There was a problem hiding this comment.
I added support for using chrome shortcuts by adding it in the manifest file but the shortcut stopped working in the chrome elements panel. Not sure if this is desired.
I also tried this feature in a recently large react app and the expand all on the root node has a small lag and the close all works well. This lag is there even in the elements panel of chrome so this should be acceptable.
frontend/Store.js
Outdated
| @@ -340,6 +340,14 @@ class Store extends EventEmitter { | |||
| this.emit(id); | |||
| } | |||
|
|
|||
| toggle(value: boolean) { | |||
There was a problem hiding this comment.
Can we give this a more descriptive name?
There was a problem hiding this comment.
will toggleAllChildrenNode be more descriptive ?
|
Updated the PR with support for chrome shortcuts. |
frontend/keyboardNav.js
Outdated
| @@ -43,6 +43,17 @@ module.exports = function keyboardNav(store: Store, win: Object): (e: DOMEvent) | |||
| return; | |||
| } | |||
| e.preventDefault(); | |||
| if ((e.altKey && direction === 'right') || (e.ctrlKey && e.altKey && direction === 'right')) { | |||
There was a problem hiding this comment.
Isn't the second condition extraneous? It seems to me that the left is always true when the right is true.
Same below.
There was a problem hiding this comment.
I added this to disallow Ctrl + Right Arrow. The only supported shortcuts are Alt(Option) + Right Arrow or Ctrl + Alt + Right Arrow
There was a problem hiding this comment.
if (e.altKey && direction === 'right') is enough to match both Alt+Right and Ctrl+Alt+Right, isn't it? And it wouldn't match Ctrl+Right.
frontend/Store.js
Outdated
| if (children && children.forEach) { | ||
| children.forEach(cid => this._toggleDeepChildren(cid, value)); | ||
| } | ||
|
|
|
This looks good, thanks! |
|
Cool, thanks a lot! What is the shortcut on Linux? |
|
Alt+Right and Alt+Left should work. |
|
(But the change is not released yet.) |
|
Is this released yet? It's been 4 months |
|
Yes 😄 Probably faster to just try it out than post 😁 |
|
Ah it's Alt+Left_Arrow and Alt+Right_Arrow |
Adds #591
Took inspiration from
_revealDeep. Not sure how efficient it is when the total nodes are large. Added shortcuts inkeyboardNavbut not sure if it is the right way to do it. This should work in all the shells.