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

[tree] add extra options to the tree search input #5566

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jun 23, 2019

  • added extra showButtons options to the tree search input.
  • allows users to navigate their matches more easily using next and previous buttons.
  • allows users to close the search input.
  • fixed incorrect implementation of the searchBox.onPrevious callback.
Screen Shot 2019-06-23 at 2 58 19 PM

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added proposal feature proposals (potential future features) core issues related to the core of the application ui/ux issues related to user interface / user experience labels Jun 23, 2019
@vince-fugnitto vince-fugnitto self-assigned this Jun 23, 2019
Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Buttons looks OK and useful
Question 1: when the tree is collapse, the search does not expand the tree to show the result
Question 2: The search is navigating through each tree item when the search does not find anything, should it stay on the current selected tree item when there is no tree item matching the search !
Also, when reaching the last item, it does not loop back , as when an item is found

@vince-fugnitto
Copy link
Member Author

@lmcbout

Question 1: when the tree is collapse, the search does not expand the tree to show the result

That is the current behavior of the tree search and consistent with vscode. Only the visible part of the tree is searchable.

Question 2: The search is navigating through each tree item when the search does not find anything,
should it stay on the current selected tree item when there is no tree item matching the search ?

When there is a match you'll see orange highlighting, if there is no match this highlighting is not present.

Also, when reaching the last item, it does not loop back , as when an item is found.

I am able to see the loop back to the first item.

@vince-fugnitto
Copy link
Member Author

@lmcbout after clarifying with me face-to-face, I'll take a look at why the explorer changes selection when there are no results and next and previous are used.

@vince-fugnitto
Copy link
Member Author

@lmcbout I updated the PR so that only if there are currently highlighted items (matches) do we enable the next and previous buttons. Could you re-test the PR for me?

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Now, the "Previous" button is searching in the same direction as the "Next" button
For the search with an invalid search option, it works fine.

@vince-fugnitto
Copy link
Member Author

Now, the "Previous" button is searching in the same direction as the "Next" button
For the search with an invalid search option, it works fine.

I fixed the issue, the original implementation was calling the incorrect method for the onPrevious callback. Now we use the selectPrevNode instead of selectNextNode.

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @vince-fugnitto

- added extra `showButtons` options to the tree search input.
- allows users to navigate their matches more easily using `next` and `previous` buttons.
- allows users to `close` the search input.
- fixed incorrect implementation of the `searchBox.onPrevious` callback.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@fangnx
Copy link

fangnx commented Jun 28, 2019

It works nicely! A useful feature ;)

@vince-fugnitto vince-fugnitto merged commit 672b697 into master Jun 28, 2019
@vince-fugnitto vince-fugnitto deleted the vf/search-tree branch June 28, 2019 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application proposal feature proposals (potential future features) ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants