-
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
Add breadcrumbs for editors #9920
Conversation
4bd07da
to
1242a57
Compare
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.
Looks really nice so far, and really nice functionality. Things mostly seem to be behaving as expecting but I have a few observations after doing an initial feature test:
- Toggling the preference to show hide doesn't seem to resize the base editor, so you get a empty bar:
After waiting several minutes, I was able to get the correct behavior, but it seems to be very finicky. For example in in this GIF, you'll see the breadcrumbs toggle ON, but when I click on the editor they disappear again:
- In VSCode, clicking on a path segment reveals the corresponding segment (and selected) and its siblings at the top level vs. in this PR, we see the path segment's parent at the top level (and the segment is unselected):
The treeview seems to have a minimum height that looks a bit odd when there are not enough elements to populate the view. In VSCode the view seems to only take up the minimum height it needs (and changes height when nodes are expanded -- up to a fixed maximum height):
Very minor: the breadcrumbs is a 24px height but the viewContainer toolbar is 22px. In VSCode they are both 22px (I only noticed it via the broken toggle mentioned above 🙂)
In VSCode, if I click on a path segment in the breadcrumbs and then click on a file inside that crumb, a new editor opens with that file in preview mode. In this PR, the file opens in normal editor mode.
76fcf37
to
86d2910
Compare
@kenneth-marut-work, thanks for taking a look. I believe I've addressed your comments.
|
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.
86d2910
to
dff955c
Compare
@msujew, thanks for pointing this out. It pointed the way to some overly elaborate focus handling logic that conflicted with the expectations of the widgets being rendered in the popups. I've simplified that and used |
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.
A couple minor comments, also as we discussed offline the WorkspaceBreadcrumbsContribution doesn't seem to be doing what it should (prefixing the breadcrumbs with WS name in multiroot). But otherwise the functionality is very impressive and is working great for me, and thanks for addressing my previous comments as well 👍
dff955c
to
34bf101
Compare
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.
Looks good to me, thanks for the fix! Nice work 🍞
a89c92d
to
0a4c8fd
Compare
@colin-grant-work do you mind rebasing the pull-request, I'll take some time to review so we can have it for |
8ca31d0
to
a8efce0
Compare
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.
a09ab56
to
ee4ac3d
Compare
packages/filesystem/src/browser/breadcrumbs/filepath-breadcrumbs-contribution.ts
Outdated
Show resolved
Hide resolved
I noticed the scrollbar-electron.mp4 |
@vince-fugnitto The easiest (and, I think, most correct) way to solve that would be to remove the scroll bar from the breadcrumb popup container and require breacrumb providers to provide their own scroll if necessary - both of the current ones show tree widgets, so they come with their own scrollbars anyway. That also avoids duplication of scroll bars in case the rendered UI happens to include a scroll bar, as all widgets would. Does that sound reasonable? |
I think that sounds reasonable yes :) |
ee4ac3d
to
8677a2d
Compare
Turns out I couldn't do that. I'd hoped the good styles were attached to widgets, but in fact they're attached to the shell, and dialogues, and now also breadcrumb popups. I think we could do that better, since I think we really just mean 'everywhere in the application,' but someone decided that that should be classed down to the shell. If you agree, @vince-fugnitto, we can probably simplify the perfect scrollbar CSS section by eliminating all of the #-referencing prefixes, but that might have unintended consequences (maybe the monaco overlays use PS and want different styles, e.g.? Perhaps best to put that off for another PR. |
55a7440
to
9fa2e3a
Compare
If CI passes and no one objects, I'll merge this Wednesday or Thursday. |
This commit adds a breadcrumbs bar to the editor widget. It shows the path to the current file and outline information as breadcrumbs. A click of breadcrumbs allows to jump to other files or to code sections. Fixes #5475 Signed-off-by: Cornelius A. Ludmann <[email protected]>
Signed-off-by: Colin Grant <[email protected]>
9fa2e3a
to
81ddeee
Compare
This PR updates #6371 and completes the remaining to-do's from that PR.
Closes #6371
From that description:
A few more details:
NavigatableWidget
s (mainly editors) that are docked in an area with a horizontal tabbar. Those breadcrumbs should contain at least the path to the file.Review checklist
Reminder for reviewers