-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Update Open Editors
widget UI
#10940
Conversation
packages/navigator/src/browser/open-editors-widget/navigator-open-editors-widget.tsx
Outdated
Show resolved
Hide resolved
@msujew, with your tabbar work merged, this should be good to go after a rebase? |
79bb933
to
bb856a2
Compare
@colin-grant-work Thanks for the reminder, I rebased my changes 👍 |
@msujew, I believe the scenario I described in my comment above is still causing trouble. I've removed my self-review request just for bookkeeping purposes - ping me when it's ready for another look. |
bb856a2
to
1d0d2b2
Compare
1d0d2b2
to
80808eb
Compare
@colin-grant-work I had time to get back to this. I removed the functional change and kept it limited to purely visual changes. In my newest change I also corrected the left padding of the tree nodes, which wasn't quite correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes and new interactions look good to me. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msujew I noticed a couple of things when quickly trying out the functionality:
- It seems as though the selection is not maintained similarly to the explorer, and vscode. In the case of the open-editors widget the selection is always passive:
open-editors-selection.mov
- I noticed we did not have ellipsis for overflowing content in the open-editors, but it seems to be a regression as it used to work (tree-view: update styling when the tree row content cannot be fully displayed #7145). Not related to your changes but I thought I'd mention it here for visibility:
@vince-fugnitto Thanks for chiming in!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The styling updates look much better 👍
I confirm that:
- the padding works well
- different file-icon themes display properly
- selection is maintained, and so is the close icon
- the close icon has consistent styling with other clickable icons present in the framework
What it does
Minor changes to focus and hovering effects for the "Open Editors" widget and tree views in general, which were missing their outlines.
How to test
Review checklist
Reminder for reviewers