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: only add default color once #2753

Merged
merged 3 commits into from
May 31, 2023
Merged

fix: only add default color once #2753

merged 3 commits into from
May 31, 2023

Conversation

pipiiiiii
Copy link
Contributor

@pipiiiiii pipiiiiii commented May 26, 2023

Types

  • 🐛 Bug Fixes

Background or solution

🤖 Generated by Copilot at 588e562

  • Fix a bug that caused wrong token colors in monaco editor by checking for the scope property of the theme settings (link)
  • Add a scope property with an empty array to the default theme settings object to match the expected format for the fillDefaults function (link)

Issue: #2686

每次初始化 theme 时,会修改 themeSetting引用,导致 https://github.com/opensumi/core/blob/main/packages/theme/src/browser/theme-data.ts#L576
这里 index 会不一致,每次颜色会错位一个
有因为scope 会被补充为 [''],所以初始化的判断逻辑改一下就可以了

Changelog

🤖 Generated by Copilot at 588e562

Fixed a bug in the workbench theme service that affected the monaco editor. Improved the default theme settings with a scope property.

@pipiiiiii pipiiiiii requested review from Aaaaash and Ricbet May 26, 2023 09:09
@opensumi opensumi bot added the 🐞 bug Something isn't working label May 26, 2023
@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e2270b4) 57.72% compared to head (41492b7) 57.72%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2753   +/-   ##
=======================================
  Coverage   57.72%   57.72%           
=======================================
  Files        1327     1327           
  Lines       83581    83587    +6     
  Branches    17378    17381    +3     
=======================================
+ Hits        48246    48253    +7     
+ Misses      32114    32113    -1     
  Partials     3221     3221           
Flag Coverage Δ
jsdom 52.72% <100.00%> (+<0.01%) ⬆️
node 16.79% <0.00%> (-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%> (ø)

... and 6 files with indirect coverage changes

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

@opensumi
Copy link
Contributor

opensumi bot commented May 30, 2023

ChatGPT Code Review:

  1. 建议修改代码中的注释,将英文的字符改为中文的字符
  2. 建议在条件语句中使用全等号(===)代替双等号(==)以避免类型转换错误。
  3. 修复了一个导致 monaco 编辑器中颜色不准确的错误,解决方案是检查主题设置对象的 scope 属性,并使用 fillDefaults 函数向默认主题设置对象添加 scope 属性。这个修复同样解决了每次切换主题时导致颜色错位的 bug。

Ricbet
Ricbet previously approved these changes May 31, 2023
@pipiiiiii pipiiiiii merged commit 7791d88 into main May 31, 2023
12 checks passed
@pipiiiiii pipiiiiii deleted the fix/theme-setting branch May 31, 2023 04:00
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.

None yet

3 participants