Introduce themable colors for resolved and unresolved comments#145230
Introduce themable colors for resolved and unresolved comments#145230alexr00 merged 4 commits intomicrosoft:mainfrom
Conversation
307d2ba to
85d7be7
Compare
|
|
85d7be7 to
73743b9
Compare
73743b9 to
f1d8d94
Compare
alexr00
left a comment
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.
| 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.
alexr00
left a comment
There was a problem hiding this comment.
I made the changes I commented on so that we can merge this before our endgame starts. Thank you for the PR!
|
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.