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

colorCustomizations: Do we need editorLinkForeground? #25474

Closed
Tyriar opened this issue Apr 26, 2017 · 6 comments
Closed

colorCustomizations: Do we need editorLinkForeground? #25474

Tyriar opened this issue Apr 26, 2017 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug themes Color theme issues verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2017

#25328

  • VSCode Version: Code - Insiders 1.12.0-insider (48ae6e3, 2017-04-26T06:32:53.505Z)
  • OS Version: Linux x64 4.10.0-19-generic
  • Extensions:
Extension Author Version
EditorConfig EditorConfig 0.9.3
lorem-ipsum Tyriar 1.0.0
sort-lines Tyriar 1.2.0
theme-sapphire Tyriar 0.1.0
vscode-svgviewer cssho 1.4.0
tslint eg2 0.12.0
git-project-manager felipecaputo 1.3.2
md-navigate jrieken 0.0.1
vscode-scss mrmlnc 0.6.2
vetur octref 0.6.3
seti-icons qinjia 0.1.3

editorLinkForeground is similar to editorActiveLinkForeground but instead of needing ctrl/cmd held with mouse over it colors the link all the time. As far as I can tell we're not using this in any theme and it could only do harm imo. Coloring the text should be the role of the textmate scopes. We already underline them, why give users the option of overriding the links?

For example, this is clearly invalid json due to the coloring:

image

It's not so clear with "editorLinkForeground": "#FFFF00":

image

Overrides coloring in comments/strings:

image

@bpasero bpasero removed their assignment Apr 27, 2017
@aeschli
Copy link
Contributor

aeschli commented Apr 27, 2017

It seems linkForeground got introduced when @sandy081 and @alexandrudima added support for more tmTheme global settings. But it seems to me 'linkForeground' got added by us as it is not used by tmTheme.

I'm also in favour of removing is. editorActiveLinkForeground is good enough.

@aeschli aeschli added this to the April 2017 milestone Apr 27, 2017
@alexdima
Copy link
Member

alexdima commented Apr 27, 2017

@joshpeng asked for it to be able to control it from his theme. @Tyriar please feel free to negotiate with @joshpeng the removal of the color. I have no strong feelings either way.

#18357 (comment)

Related discussion - #18378 i.e. @akamud might also be exercising the color in his theme.

@alexdima alexdima assigned Tyriar and unassigned alexdima Apr 27, 2017
@akamud
Copy link
Contributor

akamud commented Apr 27, 2017

I'm in favor of removing it too. We discussed this before in #18738 and why it feels weird.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 27, 2017

I don't have super strong feelings on it either, including it here means committing to it though and it does lead to slightly quirky behavior when used in themes imo.

@Tyriar Tyriar removed their assignment Apr 27, 2017
@joshpeng
Copy link
Contributor

I agree the current behavior is odd. It used to not be needed when the detected-link scope worked, but with link styling coming in on top of TM scopes how would we color them otherwise? @akamud did you find another solution to this?

@aeschli aeschli added themes Color theme issues bug Issue identified by VS Code Team member as probable bug labels Apr 28, 2017
@akamud
Copy link
Contributor

akamud commented Apr 28, 2017

No @joshpeng, my theme shows weird colors for link comments at the moment.

@mjbvz mjbvz added the verified Verification succeeded label Apr 28, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug themes Color theme issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants