Tree search-box UI change to match VS Code - #8541 #9005
Conversation
There was a problem hiding this comment.
Great work so far, this is a very nice improvement and the styling and functionality are very close. I've made a couple comments on the code, but have also noticed some minor functionality issues. Please address them and I'll take another look:
-
Collapsing the tree view when the filter is active does not hide the filter (note it should reappear when the tree reopens)
-
The 'space' character should not be allowed in the search query. (In vscode, the spacebar triggers opening a file or collapsing/expanding a node, but in Theia these actions are handled differently). Disabling the spacebar character should be sufficient
-
Pressing
ctrl+backspaceshould perform the same behavior as 'escape' (deleting all characters from filter) -
Long search queries should be truncated with the ellipsis on the left-side, and never span more than one row:
VSCode:
This branch:
| @@ -16,22 +16,31 @@ | |||
|
|
|||
| .theia-search-box { | |||
| position: absolute; | |||
| top: 0px; | |||
| right: 0px; | |||
| top: 4px; | |||
There was a problem hiding this comment.
This value seems to be a little bit off, the filter should actually sit below the titlebar:
VSCode:
This branch:
You can use the variable --theia-view-container-title-height to calculate the proper offset with the 4px padding
There was a problem hiding this comment.
Regarding this, we have a scenario that title will be shown only when the explorer have 2 views (Explorer & NPM Script). In the other case it wont be available.
Reference without title
I will make changes to support both the behaviour.
There was a problem hiding this comment.
To support both (with title and without title) behaviour with 4px, have made the parent container as position:relative, also verified there is no other impact due to that change.
I also tried with --theia-view-container-title-height, but in explorer without titlebar, search box is too down thats looking odd in UI.
|
@kenneth-marut-work - Addressed all the above mentioned issues. |
There was a problem hiding this comment.
Thanks for addressing the previous comments, I've taken another look and your changes are working nicely. I have added several new observations in the code. Please also address the following below:
- After typing in the search, then focusing elsewhere and returning back to the searchbox, keyboard strokes are not registered (it seems that typing is not registered when the search box is in focus in general)
balajiv113
force-pushed
the
BUG-8541
branch
2 times, most recently
from
43eb31a to
026d8f6
Compare
|
@kenneth-marut-work - Addressed all the above mentioned issues and also fixed the focus issue. |
|
Looking good, thanks for addressing the comments and for adding the filter tooltip 👍. Can you please address the mentioned issue above and also update the changelog, and I'll review once more. |
|
@kenneth-marut-work - Fixed the issue that you mentioned and also updated the changelog. |
Signed-off-by: Balaji V <[email protected]>
|
@kenneth-marut-work - Rebased with master and updated the conflicting changelog |
Signed-off-by: Balaji V [email protected]
What it does
The following issues are fixed as a part of this PR
The fix is to make the search-box UI consistent to VSCode UI.
The following are yet to made consistent based on feedback and suggestions,
How to test
Follow the above steps and notice the issues are fixed
Review checklist