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

fix: foreground color of the match highlights on actively focused items #2564

Merged
merged 4 commits into from
Apr 18, 2023

Conversation

wangxiaojuan
Copy link
Contributor

@wangxiaojuan wangxiaojuan commented Apr 7, 2023

Types

  • 🐛 Bug Fixes

Background or solution

fixed #2520
image
image
image

Changelog

修复浅色主题下highlights文字颜色问题

@opensumi opensumi bot added the 🐞 bug Something isn't working label Apr 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

ChatGPT Code Review:

CodeReview:

从 diff 文件来看,本次提交的变更涉及到两个文件:preferences.module.lessworkbench.theme.service.ts,以及新增了两个文件 monaco.tsindex.ts

首先分析 preferences.module.less 文件中的变更:移除了一个名为 .select_default_option_tips 的元素的颜色继承属性 color: inherit;,并添加了该元素的 hover 状态下的字体颜色,变为 color: var(--kt-selectDropdown-foreground); 。根据命名规则,可以推测该元素位于下拉框中,并且是作为提示文本来使用的。

接下来看 workbench.theme.service.ts 文件中的变更:添加了对于多个颜色属性的设置,该变更可能是支持代码编辑器中的一些用户定义主题颜色的需求。该变更涉及到对 cssVariables 变量的修改,添加了 vscodeCssVariables 变量,对于以 vscode 前缀开头的颜色属性,将其放置在 vscodeCssVariables 中,其他颜色属性则添加到 cssVariables 中。最后将两个变量分别拼接到 styleNode.innerHTML 中。

最后看新增的两个文件 monaco.tsindex.ts,其中 index.ts 中引入了 monaco.ts,并将其导出。monaco.ts 文件定义了一个 CSS Token,editorSuggesFocusHighlightForeground 表示匹配高亮在激活的目标上时的前景色。由于该 Token 的定义属于主题颜色中的自定义颜色,故将其放置在 custom 文件夹下。

根据以上分析,虽然本次提交的变更较小,但它确实是与主题颜色有关的一些修改,对于相关功能可能会有所影响,需要进一步测试。

对于问题和改进建议:

1.建议在 commit message 中明确表示该提交是与主题颜色相关的,例如 "feat(theme): update colors to support user-defined theme in code editor"。

2.对于 preferences.module.less 中的 select_default_option_tips,如果颜色移除继承属性后依然能够适配当前主题颜色,则建议移除其 hover 状态的 color 属性,以达到简化代码的目的。

3.关于 workbench.theme.service.ts 中的变更,建议在提交时添加一些更详细的注释,以方便后续开发人员的维护和修改。

4.新增加的 monaco.ts 文件中的 Token 定义具有较强的业务相关性,建议添加注释来阐述其具体含义,以方便了解相关业务逻辑的人员进行维护和修改。

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.09 ⚠️

Comparison is base (c1c06c5) 57.72% compared to head (32ae9ab) 57.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2564      +/-   ##
==========================================
- Coverage   57.72%   57.63%   -0.09%     
==========================================
  Files        1324     1327       +3     
  Lines       83544    83587      +43     
  Branches    17392    17408      +16     
==========================================
- Hits        48222    48174      -48     
- Misses      32092    32175      +83     
- Partials     3230     3238       +8     
Flag Coverage Δ
jsdom 52.65% <100.00%> (-0.09%) ⬇️
node 16.76% <36.36%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kages/theme/src/browser/workbench.theme.service.ts 71.50% <100.00%> (+0.32%) ⬆️
...ages/theme/src/common/color-tokens/custom/index.ts 100.00% <100.00%> (ø)
...ges/theme/src/common/color-tokens/custom/monaco.ts 100.00% <100.00%> (ø)

... and 42 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wangxiaojuan wangxiaojuan requested a review from erha19 April 7, 2023 10:24
@erha19 erha19 merged commit 6fa336a into main Apr 18, 2023
11 checks passed
@erha19 erha19 deleted the fix/monaco_theme branch April 18, 2023 02:02
@erha19 erha19 added this to the 2.24 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 浅色主题下的代码补全样式问题
3 participants