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

No csd linux by default #68640

Merged
merged 6 commits into from
Feb 18, 2019
Merged

No csd linux by default #68640

merged 6 commits into from
Feb 18, 2019

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Feb 13, 2019

No description provided.

@sbatten sbatten added this to the February 2019 milestone Feb 13, 2019
@sbatten sbatten self-assigned this Feb 13, 2019
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@sbatten LGTM. But please open a test plan item and assign me for it 👍

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@sbatten feedback provided

@kieferrm maybe on the wording of the notification, which currently is: "We have updated the default title bar on Linux to use the native setting. If you prefer, you can go back to the custom setting".

@@ -265,6 +265,11 @@ export function getTitleBarStyle(configurationService: IConfigurationService, en
return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647
}

const accessibilityMode = configurationService.getValue('editor.accessibilitySupport');
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten get this setting only when !isMacintosh ?

@@ -265,6 +265,11 @@ export function getTitleBarStyle(configurationService: IConfigurationService, en
return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647
}

const accessibilityMode = configurationService.getValue('editor.accessibilitySupport');
if (!isMacintosh && accessibilityMode) {
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten the value of editor.accessibilitySupport is a string, so how does your check here actually work?

run: () => {
this.storageService.store('menubar/electronFixRecommended', true, StorageScope.GLOBAL);
}
const message = nls.localize('menubar.linuxTitlebarRevertNotification', "We have updated the default title bar on Linux to use the native setting. If you prefer, you can go back to the custom setting ");
Copy link
Member

Choose a reason for hiding this comment

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

@sbatten why is there a trailing whitespace at the end of the sentence (you can go back to the custom setting )?

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@sbatten looks like you changed to show separate notifications now. LGTM with some polish:

  • "Go to Setting" => "Open Settings"
  • it looks like you always store the "Do not show again" state after showing the notification, shouldn't that be a decision that is made only by the user when clicking "Don't Show Again"?

@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

@sbatten also, I would inline the "More Info" to be a link within the notification and not have it as extra button to reduce clutter. You can use markdown style styntax for that.

@sbatten
Copy link
Member Author

sbatten commented Feb 18, 2019

@bpasero, updated to use inline link as suggested and switched back to Open Settings for consistency. I kept the behavior to not keep showing this message even without the user's input so as to not bug the user. The notifications are not critical unlike the previous notification to address an electron bug.

@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

@sbatten LGTM

@sbatten sbatten merged commit d415060 into microsoft:master Feb 18, 2019
@sbatten sbatten deleted the noCSDLinux branch February 18, 2019 17:20
sandy081 pushed a commit to vldmrkl/vscode that referenced this pull request Feb 22, 2019
Revert the default custom title bar behavior on Linux
@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

2 participants