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

Add a spec for search, #605 #3299

Merged
merged 18 commits into from
Dec 14, 2019
Merged

Conversation

KaiyuWang16
Copy link
Contributor

This is the spec for #605

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

I'd like this to be a bit more detailed on some user stories. Here's a few that I specifically want more details on:

  • using search ONLY with the keyboard
  • is this a part of the TermControl or TermApp?
  • interaction with terminal when find box is open

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved

In version 1, we want realize a case sensitive/insensitive exact text match. But we may consider the following features in version 2:

1. Add "Find" button in dropdown menu to trigger search.
Copy link
Member

Choose a reason for hiding this comment

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

As an addition (or replacement) to this point, this may also be a good feature for the command palette #2046 . You and @zadjii-msft should talk about this one 😊

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 24, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I have a lot of questions.

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 1, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Re: the new UI mockup

  1. We should almost certainly have some sort of background to the entire searchbox, to help indicate that all the controls are part of a single UI element.
  2. It seems a little tall - one row for each of the close button, the search box, and the case sensitivity chckbox means that this UI is now obscuring 3 rows of terminal content. Perhaps we could collapse it to a single row more like VsCode?
    image
  3. I would consider calling out these UI as examples of what the UI could look like, but not necessarily what it will definitely look like. Then we're less tied to content of the spec in whatever we end up implementing 😋

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

One of the big concerns I still have with this spec is regarding per-terminal search vs global search.

One of the superior features of iTerm2 is it's content search. The search comes in two variants: search from active tab and search from all tabs

(emph. added)

With the current design of a per-terminal find dialog, how do we transition to a global (search all terminals) model? I know that global search is out of scope for this first iteration, but if the find dialog is inherently tied to the terminal instance it's searching, how do we then re-use this dialog for searching many terminal instances?

There's a bunch of other comments that look like they still need answering in the text of the spec as well.

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 25, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 25, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

more notes

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 26, 2019
@DHowett-MSFT
Copy link
Contributor

This should 100% not be the expected behavior. Is copyOnSelect triggered by mouse release, or by the terminal extending its selection ranges?

@carlos-zamora
Copy link
Member

This should 100% not be the expected behavior. Is copyOnSelect triggered by mouse release, or by the terminal extending its selection ranges?

I think on mouse release so we might not have to worry about it. Nonetheless, this should be called out in the concerns section.

@cinnamon-msft
Copy link
Contributor

cinnamon-msft commented Dec 3, 2019

Regarding the UI, I think it needs some polishing. In order to follow the design standards we have in the Terminal, I've created some mockups of how I imagined the UI to look. I'd love to hear what everyone thinks.
image
Without scrollbar
image
image
With scrollbar
image
image

Hi Kayla, I wonder if you know the colors used in the Font Icon and search box background in your mock ? @cinnamon-msft

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I only have small nits.

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Just create the follow up work items and I'll approve it :) Thanks!

doc/specs/#605 - Search/spec.md Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 11, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I suppose I don't really have any more concerns with this - seems good enough for me.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 11, 2019
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

i'm going to block until the duplicated text is fixed.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 12, 2019
@KaiyuWang16 KaiyuWang16 mentioned this pull request Dec 12, 2019
20 tasks
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 14, 2019
@DHowett-MSFT DHowett-MSFT changed the title Spec for #605 search experience Add a spec for search, #605 Dec 14, 2019
@DHowett-MSFT DHowett-MSFT merged commit d95020e into master Dec 14, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/kawa/605-Search-Experice-Spec branch December 14, 2019 00:41
ghost pushed a commit that referenced this pull request Dec 17, 2019
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
This is the PR for feature Search: #605 
This PR includes the newly introduced SearchBoxControl in TermControl dir, which is the search bar for the search experience. And the codes that enable Search in Windows Terminal. 

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
The PR that migrates the Conhost search module: #3279
Spec (still actively updating): #3299
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #605 
* [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
These functionalities are included in the search experience. 
1. Search in Terminal text buffer. 
2. Automatic wrap-around. 
3. Search up or down switch by clicking different buttons.
4. Search case sensitively/insensitively by clicking a button.                                                                                                                                                S. Move the search box to the top/bottom by clicking a button. 
6. Close by clicking 'X'. 
7. Open search by ctrl + F.

When the searchbox is open, the user could still interact with the terminal by clicking the terminal input area. 

While I already have the search functionalities, currently there are still some known to-do works and I will keep updating my PR:

1. Optimize the search box UI, this includes:
                                                  1) Theme adaptation. The search box background and font color 
                                                       should change according to the theme, 
                                                  2) Add background. Currently the elements in search box are all
                                                      transparent. However, we need a background. 
                                                  3) Move button should be highlighted once clicked. 
2. Accessibility: search process should be able to performed without mouse. Once the search box is focused, the user should be able to navigate between all interactive elements on the searchbox using keyboard. 

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->

To test:
1. checkout this branch.
2. Build the project. 
3. Start Windows Terminal and press Ctrl+F
4. The search box should appear on the top right corner.
@zadjii-msft zadjii-msft mentioned this pull request Apr 15, 2020
donno2048 added a commit to donno2048/terminal that referenced this pull request Sep 28, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
This is the PR for feature Search: #605 
This PR includes the newly introduced SearchBoxControl in TermControl dir, which is the search bar for the search experience. And the codes that enable Search in Windows Terminal. 

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
The PR that migrates the Conhost search module: microsoft/terminal#3279
Spec (still actively updating): microsoft/terminal#3299
<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #605 
* [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
These functionalities are included in the search experience. 
1. Search in Terminal text buffer. 
2. Automatic wrap-around. 
3. Search up or down switch by clicking different buttons.
4. Search case sensitively/insensitively by clicking a button.                                                                                                                                                S. Move the search box to the top/bottom by clicking a button. 
6. Close by clicking 'X'. 
7. Open search by ctrl + F.

When the searchbox is open, the user could still interact with the terminal by clicking the terminal input area. 

While I already have the search functionalities, currently there are still some known to-do works and I will keep updating my PR:

1. Optimize the search box UI, this includes:
                                                  1) Theme adaptation. The search box background and font color 
                                                       should change according to the theme, 
                                                  2) Add background. Currently the elements in search box are all
                                                      transparent. However, we need a background. 
                                                  3) Move button should be highlighted once clicked. 
2. Accessibility: search process should be able to performed without mouse. Once the search box is focused, the user should be able to navigate between all interactive elements on the searchbox using keyboard. 

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->

To test:
1. checkout this branch.
2. Build the project. 
3. Start Windows Terminal and press Ctrl+F
4. The search box should appear on the top right corner.
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.

5 participants