Closed 2zqa closed 1 month ago
Three notes on the colors:
TL;DR, fine to choose either color in general; the above are just some reasons why there might not be "one true" answer for certain colors and why it's probably best to just choose a self-consistent color scheme that feels the most similar.
Thanks for the insight! I guess porting is not that "absolute" or rigid as I like to think. Well in theory it maybe could be, but that would be a pipe dream.
Oh actually the indent_guide values refer to the guides on the left of the code when it is indented which aren't shown in the screenshot. This is different from the vertical ruler color which hasn't been customized.
Regarding the ghost element changes, those modify the left sidebar hover and active colors.
@kcamcam, could you perhaps co-assign @MatthewScholefield to this PR? Although I made the branch, the rationale and actual values come from Matthew and thus I think he should have the say here. (Although I'm happy to give my opinion on things and make any suggested changes)
EDIT: If GitHub even allows multiple assignees. Otherwise, please leave it as-is.
@2zqa I added @MatthewScholefield as an assignee but I think you might have meant reviewer? Unless if someone is a contributor on the repository I can't add them as reviewers.
@MatthewScholefield thanks for pointing out the proper attributes that are being referenced. That as very helpful.
@2zqa please see my supplemental findings below:
I don't see a difference in before and after for editor.indent_guide_active
, ghost_element.hover
, or ghost_element.selected
. But like I said before the explicit value is preferred over he null
value which would (I assume) fallback to the default values of Zed, which could change.
The proposed values for ghost_element.selected
#333333
(51,51,51) don't seem closer to the original values on VS Code #2A292F
(14,57,92) and not much different from the default values on Zed #30302F
(48,48,47).
To summarize my previous comment with this one:
predictive
and element.active
look greateditor.indent_guide
, editor.indent_guide_active
, and ghost_element.hover
don't seem to be changing anything but are at least explicitly setting the default values from Zed int into the theme.ghost_element.selected
is changing the default fallback for Zed and not approximating the original values from the VS Code theme.It's possible that I'm looking at the completely wrong elements again or my machine is interpreting the values in a strange way. But unless if you can help provide screen shots and justification for the rest of proposed changes (including warning.background
). In current state of the pull request I think we could only move forward with predictive
and element.active
.
However, I'm happy to keep the pull request open if you would like to continue working on editor.indent_guide
, editor.indent_guide_active
, ghost_element.hover
, and warning.background
together.
editor.indent_guide_active
beforeeditor.indent_guide_active
afterghost_element.hover
beforeghost_element.hover
afterghost_element.selected
beforeghost_element.selected
afterghost_hover.selected
orignal
as an assignee but I think you might have meant reviewer
@kcamcam No I meant assignee, because it signals that they too are responsible for getting the PR merged. I'm kinda only the messenger here really, but I'm willing to help. The reviewer should be only you since you are the sole maintainer of this repository at the moment.
However, I'm happy to keep the pull request open if you would like to continue working [...] together
I agree on this, lets not merge unfinished work into main. Main should always be usable by end users. @MatthewScholefield, would you mind taking a look at the feedback? I'll also see if I can make any changes somewhere this week, including merging the latest changes from main into this branch.
Just getting to this. So I installed the theme from main with the latest color import and decided to rigorously recreate the overrides using exact rendered color values for simplicity even though it isn't a perfect match for VSCode (if you look closely at spots where colors overlap in VSCode, you will see their colors use alpha).
Here's the current list of overrides, pulled directly from screenshots of VSCode:
"predictive": "#6b6b6b",
"editor.document_highlight.read_background": "#37373d",
"element.hover": "#212b35",
"element.active": "#04395e",
"ghost_element.selected": "#37373d",
"ghost_element.active": "#37373d",
"hint": "#494949",
"editor.indent_guide": "#404040",
"editor.indent_guide_active": "#707070",
"title_bar.inactive_background": "#1f1f1f",
"warning.background": "#1f1f1f",
"warning.border": "#404040",
"error.background": "#1f1f1f",
"error.border": "#404040",
And below is a series of screenshots for each override compared to the latest colors merged into the repo.
predictive
editor.document_highlight.read_background
element.hover
element.active
ghost_element.selected
ghost_element.active
hint
editor.indent_guide
editor.indent_guide_active
#787670
to #707070
Before:
title_bar.inactive_background
warning.background
/ warning.border
error.background
/ error.border
Beyond this, there are a series of personal overrides that I will likely be applying on top of the above that diverge from the explicit values copied from VSCode, but I think make sense for self consistency:
"editor.document_highlight.read_background": "#ffffff04",
"element.hover": "#ffffff02",
"ghost_element.hover": "#ffffff02",
"ghost_element.selected": "#ffffff07",
"title_bar.inactive_background": "#1a1a1a"
Explanations:
editor.document_highlight.read_background
element.hover
ghost_element.hover
/ ghost_element.selected
title_bar.inactive_background
Yes, that'll be up to me (unless Matthew makes their own PR). I think I'll have time to update it tomorrow.
Had some snafu's with applying changes (forgot to rebase) but it's all good now.
Changes suggested by @MatthewScholefield. I didn't override any fields already added by @nikeee. I did notice that the suggested colors differ from Matthew and nikeee - maybe something to discuss? Considering we're porting a theme, I assume there should be only one correct answer.
Closes #3