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 border for dirty tabs #59759

Merged
merged 18 commits into from Oct 8, 2018

Conversation

Projects
None yet
2 participants
@usernamehw
Contributor

usernamehw commented Oct 1, 2018

Fixes #26284

Adds a themable top border for tabs.
Disabled in hc themes.
Disabled if tab.activeBorderTop is set.

2px border does't look very good using light themes(especially fullscreen).

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 2, 2018

@usernamehw I briefly looked into this last milestone and I think to do this right, the following is needed:

  • have a setting to enable/disable this, not everyone likes to see dirty tabs with a border so it can only be optional and disabled by default
  • a tab can be in 4 states (active focused, active unfocused, inactive focused, inactive unfocused) so this color needs to be expanded to 4 colors I think
  • instead of creating a CSS rule I would much rather like to see this added to redrawTab as style on the element directly
  • I find 3px border quite a lot, I think 2px is fine too

Thanks

@bpasero bpasero added this to the On Deck milestone Oct 2, 2018

usernamehw added some commits Oct 3, 2018

Improve border for dirty tabs
1. Make it thinner
2. Add a setting to disable it
3. Use 4 colors (activeFocused, activeUnfocused, inactiveFocused, inactiveUnfocused)
4. Move part of logic
@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 3, 2018

@bpasero

I left more feedback. Also: given this is now a setting, all our built in themes should provide a color for this so that a user that enables this setting gets a nice experience for our themes. The theme defaults are all defined in our extensions folder:

image

@@ -30,6 +30,7 @@ export interface IEditorPartOptions extends IWorkbenchEditorPartConfiguration {
export const DEFAULT_EDITOR_PART_OPTIONS: IEditorPartOptions = {
showTabs: true,
dirtyTabBorder: false,

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

Suggest: highlightModifiedTabs

@@ -938,6 +938,7 @@ export interface IWorkbenchEditorConfiguration {
export interface IWorkbenchEditorPartConfiguration {
showTabs?: boolean;
dirtyTabBorder?: boolean;

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

Suggest: highlightModifiedTabs

@@ -66,6 +66,30 @@ export const TAB_ACTIVE_BORDER_TOP = registerColor('tab.activeBorderTop', {
hc: null
}, nls.localize('tabActiveBorderTop', "Border to the top of an active tab. Tabs are the containers for editors in the editor area. Multiple tabs can be opened in one editor group. There can be multiple editor groups."));
export const TAB_DIRTY_ACTIVE_FOCUSED_BORDER = registerColor('tab.dirtyActiveFocusedBorder', {

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

To make it easier for theme authors, 3 of the 4 colors should be derived from the one main color. E.g. you can reference the main color and then apply opacity to the color to alter it. That way, a theme can only define the main color and is all set.

hc: null
}, nls.localize('tabDirtyActiveFocusedBorder', "Border on the top of dirty active tab in active editor group."));
export const TAB_DIRTY_ACTIVE_UNFOCUSED_BORDER = registerColor('tab.dirtyActiveUnfocusedBorder', {

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

I suggest tab.modified instead of tab.dirty for the colors

@@ -480,6 +480,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('showEditorTabs', "Controls whether opened editors should show in tabs or not."),
'default': true
},
'workbench.editor.dirtyTabBorder': {

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

Suggest: highlightModifiedTabs

@@ -480,6 +480,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('showEditorTabs', "Controls whether opened editors should show in tabs or not."),
'default': true
},
'workbench.editor.dirtyTabBorder': {
'type': 'boolean',
'description': nls.localize('showEditorDirtyTabBorder', "Controls whether top border is drawn on dirty file tabs or not."),

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

"Controls wether modified tabs are highlighted"

This comment has been minimized.

@usernamehw

usernamehw Oct 4, 2018

Contributor

I think some people would search for border keyword to look it up in settings. It already has highlight in setting itself; combined with border should provide more searchable result.

@@ -908,8 +909,15 @@ export class TabsTitleControl extends TitleControl {
private redrawEditorDirty(editor: IEditorInput, tabContainer: HTMLElement): void {
if (editor.isDirty()) {
addClass(tabContainer, 'dirty');
if (this.accessor.partOptions.dirtyTabBorder && !this.getColor(TAB_ACTIVE_BORDER_TOP)) {

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

Sorry for not being clear here: instead of assigning a CSS class, why not set the border color directly on the element? I wanted to avoid having a new CSS rule produced for this feature.

@@ -66,6 +66,30 @@ export const TAB_ACTIVE_BORDER_TOP = registerColor('tab.activeBorderTop', {
hc: null
}, nls.localize('tabActiveBorderTop', "Border to the top of an active tab. Tabs are the containers for editors in the editor area. Multiple tabs can be opened in one editor group. There can be multiple editor groups."));
export const TAB_DIRTY_ACTIVE_FOCUSED_BORDER = registerColor('tab.dirtyActiveFocusedBorder', {
dark: null,

This comment has been minimized.

@bpasero

bpasero Oct 4, 2018

Member

I suggest to assign a default color here for dark and light theme so that enabling the setting has a visible impact.

@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 5, 2018

@bpasero , can't make it work with box-shadow because it conflicts too much. Is there any important reason it's not using borders in the first place?

In any way, awaiting more feedback.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 5, 2018

@usernamehw actually that is a good point, why are you using box-shadow for this? Can you not use border in the same way this is done for the existing top border?

@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 5, 2018

Using 1px height div?

@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 5, 2018

It's almost working now, I need only to fix 2px shifted tab.activeBorderTop...

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 5, 2018

@usernamehw yeah maybe you can use the same element we already use for the border and make it 2px instead.

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 6, 2018

@usernamehw I was not suggesting to set a border on each tab (this will have the ugly side effect of pushing down the label within) but rather reuse the DIV that is the container of the top border already (

addClass(tabContainer, 'tab-border-top');
). Can we not utilize that one for this purpose since it is already there? Should make things much simpler.

@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 6, 2018

@bpasero, This should be close to the final version.

I commented out 2 lines that seemed redundant... Is there a reason they were there?:

tabContainer.style.removeProperty('--tab-border-bottom-color');
// ...
tabContainer.style.removeProperty('--tab-border-top-color');
@usernamehw

This comment has been minimized.

Contributor

usernamehw commented Oct 6, 2018

Should workbench.editor.highlightModifiedTabs be added to telemetry?

const configurationValueWhitelist = [
'editor.fontFamily',
'editor.fontWeight',
'editor.fontSize',
'editor.lineHeight',
'editor.letterSpacing',

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 7, 2018

@usernamehw

I commented out 2 lines that seemed redundant... Is there a reason they were there?

I think its fine to leave them, we remove a CSS variable that we added before. Its more a cleanup operation without visual impact.

Should workbench.editor.highlightModifiedTabs be added to telemetry?

Yes, why not. This list is probably out of date by now anyways, I typically forget to update it.

bpasero added some commits Oct 8, 2018

@bpasero bpasero modified the milestones: On Deck, October 2018 Oct 8, 2018

@bpasero

This comment has been minimized.

Member

bpasero commented Oct 8, 2018

Thanks, this is good to merge. I did some slight changes:

  • I think its fine to allow for both modified border color and top border color if the theme sets this. Anything else would probably make the user thing something is broken. The modified color will not win though if the active tab has a border top color specified.
  • I tweaked the defaults of the colors to apply the same dimming as for the foreground color of a tab to be in sync, let me know if that does not work for you

@bpasero bpasero merged commit e891e2f into Microsoft:master Oct 8, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Details
@bpasero

This comment has been minimized.

Member

bpasero commented Oct 8, 2018

Actually I will push another change to ensure that the dirty border always wins even if a normal border is configured. It seems to me the modified border is much more specific compared to a normal border from the theme and should always win.

@usernamehw usernamehw deleted the usernamehw:dirty branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment