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

Polish breadcrumbs icons spacing #79160

Merged
merged 2 commits into from Aug 20, 2019

Conversation

@mbelsky
Copy link
Contributor

commented Aug 15, 2019

Hey,

There is a simple fix for #79005

@mbelsky mbelsky changed the title Polish breadcrumbs icons spacing (#79005) Polish breadcrumbs icons spacing Aug 15, 2019

@jrieken
Copy link
Member

left a comment

Did you test that this doesn't have an affect to other places where these icons are being used? symbol-icon is popular, being used for quick outline and the outline tree as well.

@mbelsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I've found symbolKindToCssClass imports in these files

src/vs/editor/contrib/documentSymbols/outlineTree.ts isn't affected
src/vs/editor/standalone/browser/quickOpen/quickOutline.ts I haven't found a way to test it
src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts
src/vs/workbench/contrib/callHierarchy/browser/callHierarchyTree.ts isn't affected
🚫src/vs/workbench/contrib/quickopen/browser/gotoSymbolHandler.ts is affected. In addition .quick-open-entry-icon already sets margin-right: 4px
🚫src/vs/workbench/contrib/search/browser/openSymbolHandler.ts same as gotoSymbolHandler.ts

Well I'll find a better way to add some spacing

@MJ-Mohith

This comment has been minimized.

Copy link

commented Aug 17, 2019

It would be great if you upload a screenshot of both previous and after your change.

@mbelsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@jrieken thank you for review. I've created a special css rule to fix that issue.

I'm not sure that src/vs/workbench/browser/parts/editor/media/breadcrumbscontrol.css is a correct file for this change however I choose it because src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts renders this nodes.

@MJ-Mohith

This comment has been minimized.

Copy link

commented Aug 20, 2019

Did you test your change and see the effect when you build your instance of vscode?

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Changes look better (less risky). Thanks. Maybe move the change above line 20 as the rules in that range talk about monaco-breadcrumb-item.

@jrieken jrieken added this to the August 2019 milestone Aug 20, 2019

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

wrt the value, I feel like the padding should be 4px. It's a little hard to compare as the tree shows items in a larger font (and uses a different implementation)

@mbelsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@MJ-Mohith yes, I did

@jrieken Sure, today I'll move this rule above.

And about the padding: I've seen the tree's padding is 4px, however file monaco-breadcrumb-item has 6px padding.
Screenshot 2019-08-20 at 12 51 43

So I've selected 6px as value because it's a breadcrumb item too.

Well should I change it to 4px?

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

And about the padding: I've seen the tree's padding is 4px, however file monaco-breadcrumb-item has 6px padding.

Ah, that's where it comes from. Yeah, makes sense then. Consistency is key

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Thanks

@jrieken jrieken merged commit fe70d57 into microsoft:master Aug 20, 2019

1 of 2 checks passed

VS Code queued
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.