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
Merged

Polish breadcrumbs icons spacing #79160

merged 2 commits into from Aug 20, 2019

Conversation

mbelsky
Copy link
Contributor

@mbelsky mbelsky 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
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mbelsky 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
Copy link

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

@mbelsky
Copy link
Contributor Author

mbelsky 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
Copy link

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

@jrieken
Copy link
Member

jrieken 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
Copy link
Member

jrieken 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
Copy link
Contributor Author

mbelsky 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
Copy link
Member

jrieken 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
Copy link
Member

jrieken commented Aug 20, 2019

Thanks

@jrieken jrieken merged commit fe70d57 into microsoft:master Aug 20, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants