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

Feature: tweak search behaviour #353

Merged
merged 14 commits into from
Jul 27, 2019

Conversation

fanny
Copy link
Contributor

@fanny fanny commented Jul 25, 2019

This pr closes #301

The strategy used here was: Get the first index where elementID is larger than the selected element and reorder the search results for these elements so that the range with the largest is at the beginning of the matrix and the range with the smallest in the end.

@fanny fanny force-pushed the feat/tweak-search-behaviour branch from 7012679 to 05455d1 Compare July 25, 2019 15:51
@fanny
Copy link
Contributor Author

fanny commented Jul 26, 2019

I'm sorry for these many commits, I got a little lost on how I could fix an error.

@bvaughn
Copy link
Owner

bvaughn commented Jul 26, 2019

No worries. This looks nice. I'll try to find the time to test it soonish.

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This is a nice UX tweak 😄 Thanks for contributing!

searchIndex = Math.min(
((prevSearchIndex: any): number),
searchResults.length - 1
);
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I think we might want to preserve this behavior since it would be less of a jump in some cases.

  1. Go to the demo page
  2. Search for "ModernContext" and hit enter twice to select the third result
  3. Type an additional "." (so you're now searching for "ModernContext.")

Before this change, the selected index would go from 3 to 2 (the nearest result given the updated text):
search-before-Kapture 2019-07-27 at 8 50 10

After this change, the cursor will jump back to index 0 (the furthest result).
search-after-Kapture 2019-07-27 at 8 51 35

To be fair, this is a minor thing- but I think it's slightly nicer if we preserve the original behavior for this case. I think we can do that with just a minor tweak.

diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js
index 5af9398..17b1a5c 100644
--- a/src/devtools/views/Components/TreeContext.js
+++ b/src/devtools/views/Components/TreeContext.js
@@ -277,6 +277,7 @@ function reduceSearchState(store: Store, state: State, action: Action): State {
     selectedElementIndex,
   } = state;
 
+  const prevSearchIndex = searchIndex;
   const prevSearchText = searchText;
   const numPrevSearchResults = searchResults.length;
 
@@ -380,10 +381,20 @@ function reduceSearchState(store: Store, state: State, action: Action): State {
             recursivelySearchTree(store, rootID, regExp, searchResults);
           });
           if (searchResults.length > 0) {
-            if (selectedElementID !== null) {
-              searchIndex = getNearestResult(searchResults, selectedElementID);
+            if (prevSearchIndex === null) {
+              if (selectedElementID !== null) {
+                searchIndex = getNearestResult(
+                  searchResults,
+                  selectedElementID
+                );
+              } else {
+                searchIndex = 0;
+              }
             } else {
-              searchIndex = 0;
+              searchIndex = Math.min(
+                ((prevSearchIndex: any): number),
+                searchResults.length - 1
+              );
             }
           }
         }

Since I'm already in the code I'll just go ahead and make this tweak. If you know of a good reason why we shouldn't do this though, let me know?

bvaughn pushed a commit that referenced this pull request Jul 27, 2019
Merge PR #353 from @fanny

This change changes search beahvior to initially select the result nearest the currently selected element (rather than selecting the first result in the set).
@bvaughn
Copy link
Owner

bvaughn commented Jul 27, 2019

Squash merged via 3a3187f

GitHub isn't picking up on the merge, so I'm going to manually close this PR. Thanks again for contributing!

@bvaughn bvaughn closed this Jul 27, 2019
@bvaughn bvaughn reopened this Jul 27, 2019
@bvaughn
Copy link
Owner

bvaughn commented Jul 27, 2019

I think the way I merged messed up attributing the fix to you, so I force-pushed master and I'm going to merge through the GitHub UI 😄 I'll just apply my tweak after it's merged.

@bvaughn bvaughn merged commit 29a6bf2 into bvaughn:master Jul 27, 2019
selectedElementID: number | null
) {
const result = searchResults.findIndex(
value => value >= ((selectedElementID: any): number)
Copy link
Owner

Choose a reason for hiding this comment

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

Whoops! I overlooked something 😁

We shouldn't be comparing IDs here, but indices. These won't necessarily correlate. I'll make a follow up fix!

@bvaughn
Copy link
Owner

bvaughn commented Jul 27, 2019

Ok, index change made. Let me know if you see anything funky~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweak search behavior
2 participants