Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Make alt click expand/collapse all the nodes#4309

Merged
bomsy merged 2 commits into
firefox-devtools:masterfrom
Fischer-L:4061-alt-click-expand-all
Oct 10, 2017
Merged

Make alt click expand/collapse all the nodes#4309
bomsy merged 2 commits into
firefox-devtools:masterfrom
Fischer-L:4061-alt-click-expand-all

Conversation

@Fischer-L

Copy link
Copy Markdown
Contributor

Associated Issue: #4061

Summary of Changes

Expand/Collapse all nodes in the source tree when doing alt click on the tree item(folder)

Screenshots

4061-altkey-expand-collapse-all-nodes

@jasonLaster

Copy link
Copy Markdown
Contributor

Nice @Fischer-L!
@amitzur?

@amitzur

amitzur commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

I didn't know this feature existed in other places like finder and chrome!
Re UX I think it's proper to back this by a button, possibly in a top bar on the right pane, where a "scroll from source" button can also exist. Just a thought

@Fischer-L

Fischer-L commented Oct 7, 2017

Copy link
Copy Markdown
Contributor Author

@amitzur

I didn't know this feature existed in other places like finder and chrome!
Re UX I think it's proper to back this by a button, possibly in a top bar on the right pane, where a "scroll from source" button can also exist. Just a thought

Yes this alt click feature exists on Chrome's devtool, finder and Sublime, which is handy.

Comment thread src/components/shared/ManagedTree.js Outdated
}

setExpanded = (item: Item, isExpanded: boolean) => {
setExpanded = (item: Item, isExpanded: boolean, isRecursive: boolean) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm thinking something like shouldIncludeChildren feels better and clearer then isRecursive

Comment thread src/components/shared/ManagedTree.js Outdated
expanded.delete(itemPath);
}
if (isRecursive) {
let children = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can remove this line ...

Comment thread src/components/shared/ManagedTree.js Outdated
let children = null;
let parents = [item];
while (parents.length) {
children = [];

@bomsy bomsy Oct 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... and make this const children = [];

Comment thread src/components/shared/ManagedTree.js Outdated
expanded.add(itemPath);
} else {
expanded.delete(itemPath);
}

@bomsy bomsy Oct 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could pull this into a local function like this here

    const toggleItem = item => {
      const path = this.props.getPath(item);
      if (isExpanded) {
        expanded.add(path);
      } else {
        expanded.delete(path);
      }
    };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then do toggleItem(item); here

}
children.push(child);
}
}

@bomsy bomsy Oct 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This for loop can become

        for (const parent of parents) {
          if (parent.contents && parent.contents.length) {
            for (const child of parent.contents) {
              toggleItem(child);
              children.push(child);
            }
          }
        }

@bomsy bomsy Oct 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As you should not need the continue ...

@bomsy bomsy Oct 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And may not need nodeHasChildren since you only use parent.contents anyway, you can just check that

Comment thread src/components/shared/ManagedTree.js Outdated
@@ -1,5 +1,6 @@
// @flow
import React, { createFactory, Component } from "react";
import { nodeHasChildren } from "../../utils/sources-tree";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can remove this line

@bomsy

bomsy commented Oct 9, 2017

Copy link
Copy Markdown
Contributor

@Fischer-L Great Stuff here! left some comments..

@bomsy bomsy force-pushed the 4061-alt-click-expand-all branch from 807c0e1 to a6e6e37 Compare October 10, 2017 21:37
@bomsy

bomsy commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

@Fischer-L i fixed up the few comments so we could landing this ...

Thanks for the amazing Work!!!!

@bomsy bomsy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@bomsy bomsy merged commit 37b9608 into firefox-devtools:master Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants