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

Added support for custom icons in various editor inputs #200329

Merged
merged 19 commits into from
Dec 11, 2023

Conversation

benibenj
Copy link
Contributor

@benibenj benibenj commented Dec 8, 2023

This PR introduces the ability to use codicons in various editor inputs. The changes include modifications to the editor input classes to accept a ThemeIcon object, which can be used to set a custom icon.

#199965
#200147

@benibenj benibenj self-assigned this Dec 8, 2023
@benibenj benibenj requested a review from bpasero December 8, 2023 11:33
@benibenj benibenj marked this pull request as ready for review December 8, 2023 11:33
@benibenj benibenj added this to the December / January 2024 milestone Dec 8, 2023
src/vs/workbench/browser/labels.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/iconLabel/iconlabel.css Outdated Show resolved Hide resolved
@bpasero
Copy link
Member

bpasero commented Dec 8, 2023

Great small change, here are more editors to consider adopting:

  • WorkspaceTrustEditorInput
  • WalkThroughInput
  • GettingStartedInput
  • TerminalEditorInput
  • RuntimeExtensionsInput
  • DissassemblyViewInput
  • ChatEditorInput

@Tyriar
Copy link
Member

Tyriar commented Dec 8, 2023

If you do TerminalEditorInput, note that the icon (codicon or uri) and its color is currently dynamic (driven via CSS):

override getLabelExtraClasses(): string[] {
if (!this._terminalInstance) {
return [];
}
const extraClasses: string[] = ['terminal-tab'];
const colorClass = getColorClass(this._terminalInstance);
if (colorClass) {
extraClasses.push(colorClass);
}
const uriClasses = getUriClasses(this._terminalInstance, this._themeService.getColorTheme().type);
if (uriClasses) {
extraClasses.push(...uriClasses);
}
if (ThemeIcon.isThemeIcon(this._terminalInstance.icon)) {
extraClasses.push(`codicon-${this._terminalInstance.icon.id}`);
}
return extraClasses;
}

@benibenj
Copy link
Contributor Author

Great small change, here are more editors to consider adopting:

  • WorkspaceTrustEditorInput
  • WalkThroughInput
  • GettingStartedInput
  • TerminalEditorInput
  • RuntimeExtensionsInput
  • DissassemblyViewInput
  • ChatEditorInput

Added support for all EditorInputs except WalkThroughInput and GettingStartedInput as these set an svg for which we do not have a codicon.

@benibenj
Copy link
Contributor Author

I only partially supported TerminalEditorInput as I am not sure how to support URI icons. Also, I don't support changing the colour of the icon provided by an EditorInput yet which I am planning on looking into in a follow up PR.

@benibenj benibenj requested a review from bpasero December 10, 2023 11:57
@benibenj benibenj requested a review from bpasero December 11, 2023 10:07
bpasero
bpasero previously approved these changes Dec 11, 2023
bpasero
bpasero previously approved these changes Dec 11, 2023
@benibenj benibenj merged commit 66fda5f into main Dec 11, 2023
5 checks passed
@benibenj benibenj deleted the benibenj/codIconLabelSupport branch December 11, 2023 15:33
@Tyriar
Copy link
Member

Tyriar commented Dec 11, 2023

If you don't do color yet we should move the terminal back to the old system until then as it would be a regression

@benibenj
Copy link
Contributor Author

I will be looking into icon theme colours tomorrow. For the TerminalEditorInput, the icon colours should still be working after this PR as the colour classes are still being added to the extra classes as before. If it does not work then it's due to changing the CSS classes which are added to the div. In that case I'll just need to make a small adaption.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
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.

4 participants