Introduce themable colors for resolved and unresolved comments #145230
Conversation
hermannloose
force-pushed
the
comment-state-visualization
branch
from
307d2ba to
85d7be7
Compare
|
|
hermannloose
force-pushed
the
comment-state-visualization
branch
from
85d7be7 to
73743b9
Compare
hermannloose
force-pushed
the
comment-state-visualization
branch
from
73743b9 to
f1d8d94
Compare
There was a problem hiding this comment.
Looks and works great! I left a few comments, but I intend to push changes to address them today so that I can merge this before we start our endgame next week.
After our endgame week we'll do a UX review, try to get feedback from other extension authors, and work towards finalizing the API.
| } | ||
|
|
||
| set state(newState: vscode.CommentThreadState) { | ||
| this._state = newState; |
There was a problem hiding this comment.
Somewhere in this file we need to have a call to checkProposedApiEnabled, probably in set state to enforce our API proposal mechanism.
| @@ -57,6 +57,21 @@ const COLLAPSE_ACTION_CLASS = 'expand-review-action ' + ThemeIcon.asClassName(co | |||
| const COMMENT_SCHEME = 'comment'; | |||
|
|
|||
|
|
|||
| export const resolvedCommentBorder = registerColor('resolvedComment.border', { dark: Color.fromHex('#c5c5c5'), light: Color.fromHex('#555'), hc: contrastBorder }, nls.localize('resolvedCommentBorder', 'Color of borders and arrow for resolved comments.')); | |||
There was a problem hiding this comment.
I'm leaning towards calling these 'comments.resolved.border' and 'comments.unresolved.border', respectively.
There was a problem hiding this comment.
Note for myself for later: we might need to registerThemingParticipant.
There was a problem hiding this comment.
registerThemingParticipant is not needed because you're using a CSS variable 👌
| const commentThreadStateColorVar = '--comment-thread-state-color'; | ||
|
|
||
| function getCommentThreadStateColor(thread: languages.CommentThread, theme: IColorTheme): Color | undefined { | ||
| const colorId = thread.state !== undefined ? commentThreadStateColors.get(thread.state) : undefined; |
There was a problem hiding this comment.
When the thread.state is undefined, we should use the peekViewBorder.
|
There's an outage that's preventing me from merging this right now, but I will merge it when that's over. |
| const colorId = thread.state !== undefined ? commentThreadStateColors.get(thread.state) : undefined; | ||
| return colorId !== undefined ? theme.getColor(colorId) : undefined; | ||
| function getCommentThreadWidgetStateColor(thread: languages.CommentThread, theme: IColorTheme): Color | undefined { | ||
| return getCommentThreadStateColor(thread, theme) ?? theme.getColor(peekViewBorder); |
There was a problem hiding this comment.
Should this maybe just be inlined in getCommentThreadStateColor()? Having two separate methods leads to slightly confusing naming for me, which seems like a code smell, especially since we won't use getCommentThreadStateColor() independently of getCommentThreadStateColor() from what I can see. Not a very strong opinion though.
There was a problem hiding this comment.
I split it out in preparation for including the color in the "Comments" view. There, we will not use the peek view color, but I do expect that we still use the same resolved/unresolved colors.
|
I just realized I had forgotten to upload the local changes I had made yesterday to base the unresolved colors on |
Implementation proposal for #127473.