Adding navigation button to Search Input. by wldcordeiro · Pull Request #2463 · firefox-devtools/debugger

wldcordeiro · 2017-03-27T08:15:05Z

Associated Issue: #2387

The previous PR was made against master and github got confused and closed it when I rebased. I'm reopening on behalf of @nadavoran with some tweaks and fixes to pass CI.

Summary of Changes

Add the up/down arrow navigation to the SearchInput and use in file search (@nadavoran's original work)
Tweak the original PR to pass lint/CI, add the navigation to project search

Test Plan

Command-P opens the project search
type a search with multiple results, click the next/prev arrows to traverse
type a search with one result and see no arrows
try the same with file search (both with symbol searching and code searching.)

Screenshots/Videos

wldcordeiro · 2017-03-27T08:25:01Z

+      Svg("arrow-up", {
+        className: classnames("nav-btn", "prev"),
+        onClick: handlePrev,
+        title: "Previous Result",


Could put these in L10N.

codecov · 2017-03-27T08:43:34Z

Codecov Report

Merging #2463 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2463   +/-   ##
=======================================
  Coverage   54.07%   54.07%           
=======================================
  Files          51       51           
  Lines        1986     1986           
  Branches      398      398           
=======================================
  Hits         1074     1074           
  Misses        912      912

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1e196f...5e40a3b. Read the comment docs.

jasonLaster

Thanks for picking this up.

One highlevel point, I think next/prev makes more sense for text search and less sense for project and function search which are list based. The rationale is that scrolling through a list is very efficient, whereas jumping to the next item in a file can be trickier.

jasonLaster · 2017-03-27T20:46:22Z

+        }
+        this.setState({ selectedResultIndex: nextResultIndex });
+        this.onSelectResultItem(searchResults[nextResultIndex]);
+      }


Hmm - I'm a bit confused by this logic.

It looks very similar to what we're doing in search search to determine the next index. Is there an easy way we can unify it?

jasonLaster · 2017-03-27T20:46:39Z

+        nextResultIndex = 0;
+      }
+      this.setState({ selectedResultIndex: nextResultIndex });
+      this.onSelectResultItem(searchResults[nextResultIndex]);


This looks like it can be shared too...

jasonLaster · 2017-03-27T20:49:19Z

+        title: "Previous Result",
+      })
+    );
+  },


SearchInput could be a good component to snapshot test:

https://facebook.github.io/jest/docs/snapshot-testing.html#content

jasonLaster · 2017-03-27T20:50:01Z

+        className: classnames("nav-btn", "next"),
+        onClick: handleNext,
+        title: "Next Result",
+      }),


what's our preference with wrapping - next at the end and previous at 0?

jasonLaster · 2017-04-03T20:22:42Z

This looks much better, I'll review soon!

jasonLaster · 2017-04-04T01:50:05Z

Rebased and it works great.

Here's a GIF to see how great it is:

jasonLaster

Made some small tweaks, but lets land this and we can always polish later!

wldcordeiro · 2017-04-05T22:32:02Z

Working on a rebase/polish commit now. Hopefully will push today (vacation is slowing me down 😆)

jasonLaster · 2017-04-05T22:33:13Z

Hehe. Thanks. I was so close to getting this in yesterday!

…

On Wed, Apr 5, 2017 at 6:32 PM Wellington Cordeiro ***@***.***> wrote: Working on a rebase/polish commit now. Hopefully will push today (vacation is slowing me down 😆) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2463 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPiYvziquNHGIgN-sqoPpNr2-PrVaNmks5rtBZjgaJpZM4Mp7Ry> .

jasonLaster

Thanks. Looking better

jasonLaster · 2017-04-06T13:30:12Z

        onChange: this.onChange,
        onKeyUp: this.onKeyUp,
        onKeyDown: this.onKeyDown,
+        handleNext: e =>


hmm, not sure if we need the ternary. if we're not rendering the buttons will these be used?

jasonLaster · 2017-04-06T13:30:31Z

  }

+  renderNav() {
+    const { count, handleNext, handlePrev } = this.props;


lets put the feature flag here

Oh yeah, forgot to feature flag it. 😆

nadavoran · 2017-04-06T13:48:00Z

Wow that looks great, thanks for adding this and all your work to make it so nice

jasonLaster · 2017-04-06T23:28:34Z

Thanks @nadavoran for getting this started!
Thanks @wldcordeiro!

* Adding navigation button to Search Input. re firefox-devtools#2387 * Only show search navigation for file text search. * Reduce some redundancy in SearchBar for traversing results. * Feature flag the search navigation buttons and remove redundant ternary.

wldcordeiro force-pushed the search-nav-buttons branch from 92452f1 to cb1ce04 Compare March 27, 2017 08:20

wldcordeiro commented Mar 27, 2017

View reviewed changes

jasonLaster reviewed Mar 27, 2017

View reviewed changes

wldcordeiro force-pushed the search-nav-buttons branch from cb1ce04 to 031d644 Compare March 29, 2017 22:04

jasonLaster force-pushed the search-nav-buttons branch from 031d644 to 27afe5f Compare April 4, 2017 01:49

jasonLaster force-pushed the search-nav-buttons branch 2 times, most recently from ee36e2f to fd94386 Compare April 4, 2017 20:40

jasonLaster approved these changes Apr 4, 2017

View reviewed changes

Adding navigation button to Search Input. re firefox-devtools#2387

382c149

wldcordeiro force-pushed the search-nav-buttons branch from fd94386 to 382c149 Compare April 5, 2017 22:38

wldcordeiro added 2 commits April 5, 2017 16:45

Only show search navigation for file text search.

14008e7

Reduce some redundancy in SearchBar for traversing results.

6ec3d7a

jasonLaster reviewed Apr 6, 2017

View reviewed changes

Feature flag the search navigation buttons and remove redundant ternary.

5e40a3b

jasonLaster merged commit 896c1fd into firefox-devtools:master Apr 6, 2017

wldcordeiro deleted the search-nav-buttons branch April 6, 2017 23:31

jasonLaster mentioned this pull request Apr 7, 2017

[SearchBar] Polish search buttons #2589

Closed

3 tasks

Uh oh!

Conversation

wldcordeiro commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Test Plan

Screenshots/Videos

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Apr 3, 2017

Uh oh!

jasonLaster commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

wldcordeiro commented Apr 5, 2017

Uh oh!

jasonLaster commented Apr 5, 2017 via email

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadavoran commented Apr 6, 2017

Uh oh!

jasonLaster commented Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wldcordeiro commented Mar 27, 2017 •

edited

Loading

codecov Bot commented Mar 27, 2017 •

edited

Loading

jasonLaster commented Apr 4, 2017 •

edited

Loading