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

[outline] Enable filtering the outline list #7088

Merged
merged 15 commits into from Oct 15, 2018

Conversation

Projects
None yet
4 participants
@kenjiO
Contributor

kenjiO commented Oct 2, 2018

Fixes #7054

Summary of Changes

  • Add react component for an input box to filter the outline items
  • Add code to the Outline component that filters based on the input in the new component

Test Plan

  • manual testing
  • automated tests

screen shot 2018-10-02 at 3 19 54 pm
screen shot 2018-10-02 at 3 20 11 pm

@kenjiO kenjiO requested a review from flodolo as a code owner Oct 2, 2018

@kenjiO

This comment has been minimized.

Contributor

kenjiO commented Oct 2, 2018

Can you let me know how this looks so far? I am still planning on writing some tests and maybe doing some small changes.

constructor(props: Props) {
super(props);
(this: any).filterOutlineItems = this.filterOutlineItems.bind(this);
(this: any).onUpdateFilter = this.onUpdateFilter.bind(this);

This comment has been minimized.

@kenjiO

kenjiO Oct 2, 2018

Contributor

If we use the ES6 arrow function definitions for filterOutlineItems and onUpdateFilter we wouldn't need to bind them here. Do you want to do this? If so do you want to change all the function definitions in the class to this style for consistancy?

@@ -606,6 +606,10 @@ sources.header=Sources
# LOCALIZATION NOTE (outline.header): Outline left sidebar header
outline.header=Outline
# LOCALIZATION NOTE (outline.placeholder): Placeholder text for the filter input
# input element

This comment has been minimized.

@flodolo

flodolo Oct 3, 2018

nit: you have two "input" in the comment

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 3, 2018

I love this @kenjiO ! Awesome work! 👍

@darkwing darkwing added the pr-wip label Oct 5, 2018

if (e.key === "Escape" && this.props.filter !== "") {
e.preventDefault();
this.props.updateFilter("");
}

This comment has been minimized.

@kenjiO

kenjiO Oct 9, 2018

Contributor

I like having the user be able to press escape to clear the filter when it is in focus. There is an issue that escape already opens/closes the split console. I made it so if the user presses escape it clears the filter but doesn't toggle the split console. Then once the filter is cleared pressing escape furthermore toggles the split console. Is this OK to do?

This comment has been minimized.

@AnshulMalik

AnshulMalik Oct 10, 2018

Member

That sounds right to me

@AnshulMalik

This comment has been minimized.

Member

AnshulMalik commented Oct 10, 2018

This is awesome :)

@darkwing darkwing added testing pr-wip and removed pr-wip labels Oct 10, 2018

kenjiO added some commits Oct 10, 2018

@kenjiO

This comment has been minimized.

Contributor

kenjiO commented Oct 10, 2018

@darkwing I've finished getting this to filter the list by function names. At the call we discussed also filtering by class name but I don't know what the desired behavior for that would be. For example if the filter matched a class name would you show all the functions in that class or just show the class name?

@darkwing

This comment has been minimized.

Contributor

darkwing commented Oct 10, 2018

Wow, that's a great question. Let's focus on functions only for now, with the caveat that if the function was a class method, we should show the class name above it, IMO. Thoughts?

@darkwing darkwing changed the title from WIP [outline] Enable filtering the outline list to [outline] Enable filtering the outline list Oct 11, 2018

@kenjiO

This comment has been minimized.

Contributor

kenjiO commented Oct 11, 2018

@darkwing I agree at keeping it this way now. Usually there are not that many classes defined in a file so I don't think being able to filter them will be that useful. Currently if any class functions match the filter it will show all the class names with the matching functions underneath their associated class. I don't have any other pending changes for this PR so you can merge it if you don't need any further changes.

@darkwing darkwing merged commit fe06d1b into devtools-html:master Oct 15, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

egdbear pushed a commit to egdbear/debugger.html that referenced this pull request Oct 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment