Programming exercises: Add custom themes for the Monaco editor
#9463
Conversation
github-actions
bot
added
tests
client
pzdr7
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces several new interfaces and constants related to theming in the Monaco editor. It adds the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (10)
- src/main/webapp/app/shared/monaco-editor/model/themes/editor-colors.interface.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/model/themes/monaco-dark.theme.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/model/themes/monaco-theme-definition.interface.ts (1 hunks)
- src/main/webapp/app/shared/monaco-editor/monaco-editor.component.scss (1 hunks)
- src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (3 hunks)
- src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-theme.spec.ts (1 hunks)
- src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/main/webapp/app/shared/monaco-editor/model/themes/editor-colors.interface.ts (1)
src/main/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.ts (1)
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-dark.theme.ts (1)
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts (1)
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts (1)
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-theme-definition.interface.ts (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-theme.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 Biome
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts
[error] 51-51: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (24)
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-theme-definition.interface.ts (2)
1-2: LGTM: Imports are correct and follow guidelines.The imports are necessary for the interface definition and follow the Angular style guidelines. The use of PascalCase for imported types is correct as per the coding guidelines.
1-9: Overall assessment: Well-structured and aligned with PR objectives.This new file introduces the
MonacoThemeDefinitioninterface, which is a crucial part of implementing custom themes for the Monaco editor as per the PR objectives. The code is well-structured, follows the coding guidelines, and promotes code reuse. The suggestion to add JSDoc comments would further enhance the code quality and maintainability.src/main/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.ts (4)
5-27: LGTM: Interface naming and structureThe
LanguageTokenStyleDefinitioninterface is well-structured and follows the PascalCase naming convention as per the coding guidelines. The interface clearly defines the properties needed for styling language tokens in the Monaco editor.
9-9: LGTM: Property naming and typesThe properties within the
LanguageTokenStyleDefinitioninterface are well-defined:
- All properties use camelCase naming as per the guidelines.
- Types are appropriate for each property (string for colors and token, union type for fontStyle).
- Optional properties (languageId, foregroundColor, backgroundColor, fontStyle) are correctly marked with '?'.
This structure provides flexibility in defining token styles while maintaining type safety.
Also applies to: 14-14, 18-18, 22-22, 26-26
1-27: Summary: Well-implemented interface for custom Monaco editor themesThis new
LanguageTokenStyleDefinitioninterface is a solid addition to the project, directly supporting the PR's objective of adding custom themes for the Monaco editor. It provides a clear and type-safe structure for defining token styles, which is crucial for implementing custom syntax highlighting.Key points:
- The interface is well-structured and follows TypeScript and Angular best practices.
- Property naming and types are appropriate and consistent with the coding guidelines.
- JSDoc comments provide good documentation, with a minor suggestion for improvement.
- The implementation aligns well with the PR's goal of enhancing the Monaco editor's styling capabilities.
Overall, this file makes a valuable contribution to the custom theming feature, providing a robust foundation for defining language-specific token styles.
1-27: Verify indentation and consider using a formatting toolThe code generally adheres well to the provided coding guidelines. However, it's important to ensure consistent indentation throughout the project.
Please confirm that the indentation is set to 4 spaces, as specified in the coding guidelines. To maintain consistency across the project, consider using a formatting tool like Prettier with a configuration that matches your project's coding style. This will help automate formatting and ensure consistency across all files.
If the above command returns matches, it confirms that 4-space indentation is being used as expected.
✅ Verification successful
Adjusting the indentation verification script
The previous command failed because
typescriptis not a recognized file type. Let's target.tsfiles directly to verify 4-space indentation.
Indentation verified as consistent with 4 spaces
The indentation in
language-token-style-definition.interface.tsadheres to the project’s coding guidelines, consistently using 4 spaces throughout the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify indentation in the file rg --type typescript -A 1 '^\s{4}[^\s]' src/main/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.tsLength of output: 175
Script:
#!/bin/bash # Verify 4-space indentation in the TypeScript file rg -g "*.ts" "^ {4}\S" src/main/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.tsLength of output: 316
src/main/webapp/app/shared/monaco-editor/monaco-editor.component.scss (1)
Line range hint
1-62: Consider expanding theme-related modificationsWhile the current change addresses a specific styling concern, it doesn't fully align with the PR objectives of adding custom themes for the Monaco editor.
Consider expanding on this change to incorporate more theme-related modifications:
- Introduce CSS variables for theme-specific colors and styles.
- Use these variables throughout the file to enable easy theme switching.
- Create separate theme files (e.g.,
monaco-editor-light-theme.scssandmonaco-editor-dark-theme.scss) to define theme-specific variables.Example:
// In monaco-editor.component.scss .monaco-editor-container { .monaco-editor { background-color: var(--monaco-editor-background); color: var(--monaco-editor-foreground); // ... other theme-related styles } } // In monaco-editor-light-theme.scss :root { --monaco-editor-background: #ffffff; --monaco-editor-foreground: #000000; // ... other light theme variables } // In monaco-editor-dark-theme.scss :root { --monaco-editor-background: #1e1e1e; --monaco-editor-foreground: #d4d4d4; // ... other dark theme variables }This approach would provide a more comprehensive theming solution, aligning better with the PR objectives.
Let's check if there are any existing theme-related files or variables in the project:
#!/bin/bash # Search for theme-related files and variables rg --type css -i '(theme|light|dark)' src/ fd -e scss -e css . src/ | grep -i 'theme'src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts (2)
1-1: LGTM: Import statement is correct and follows guidelines.The import statement correctly imports the
MonacoThemeDefinitioninterface using a relative path, which is appropriate for importing from within the same module.
3-5: LGTM: Constant declaration and basic theme properties are well-defined.The constant
MONACO_LIGHT_THEME_DEFINITIONis correctly exported and typed. The theme id 'custom-light' is descriptive and follows camelCase naming convention. The use of 'vs' as the base theme is appropriate for a light theme.src/main/webapp/app/shared/monaco-editor/model/themes/monaco-dark.theme.ts (3)
1-5: LGTM: Import and constant declaration look good.The import statement and constant declaration follow Angular style guidelines and TypeScript best practices. The constant name is in uppercase, and the type is properly specified.
24-46: Great job on editor colors, especially diff styling!The editor colors are well-defined and provide good contrast for readability. The diff colors, in particular, align well with the PR objective of making the diff editor resemble GitHub's styling. The use of alpha values for subtle highlighting is a nice touch.
This implementation successfully addresses the PR's goal of adjusting the diff editor's theme to resemble GitHub's styling more closely.
1-47: Excellent implementation of the custom dark theme for Monaco editor!This file successfully implements a custom dark theme for the Monaco editor, aligning perfectly with the PR objectives. The theme definition is comprehensive, covering both token styles and editor colors, with special attention to diff styling that resembles GitHub's approach.
Key points:
- The code is well-structured and follows TypeScript and Angular conventions.
- It adheres to the provided coding guidelines, including naming conventions and type specifications.
- The color scheme provides good contrast and readability, especially for diff views.
The only suggestion for improvement is to consider extracting color constants for better maintainability, as mentioned in a previous comment.
Overall, this implementation effectively enhances the styling capabilities of the Monaco editor, meeting the primary objective of the PR.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-theme.spec.ts (2)
7-36: Well-structured test setup with comprehensive theme definition.The test setup is well-organized and covers various aspects of the theme, including color definitions and token styles. The explicit use of
undefinedfor removed line backgrounds is a good practice for testing edge cases.The use of TypeScript interfaces (
EditorColors,LanguageTokenStyleDefinition,MonacoThemeDefinition) enhances type safety and code readability.
38-66: Well-implemented test case for theme registration.The test case effectively verifies the correct registration of the theme using Jest spies and specific assertions. The use of
toHaveBeenCalledExactlyOnceWithensures that the theme is registered with the exact expected parameters, which is crucial for detecting any unintended changes in the theme definition.The test follows Jest best practices and provides good coverage for the happy path of theme registration.
src/main/webapp/app/shared/monaco-editor/model/themes/editor-colors.interface.ts (1)
7-14: LGTM! Well-documented basic color properties.The basic editor color properties are well-defined and documented. The use of optional properties allows for flexible theme definitions. The property names follow the camelCase convention as required by the coding guidelines.
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (3)
9-10: LGTM: New theme definition imports.The new imports for
MONACO_LIGHT_THEME_DEFINITIONandMONACO_DARK_THEME_DEFINITIONare correctly added and will be used in the updated test assertions.
45-45: LGTM: Updated assertion for light theme.The assertion now correctly uses
MONACO_LIGHT_THEME_DEFINITION.id, improving maintainability and adhering to the coding guidelines. The use oftoHaveBeenCalledExactlyOnceWithis appropriate for this test case.
Line range hint
1-71: Overall assessment: Changes improve test maintainability.The updates to this test file align well with the PR objectives by incorporating the new theme definitions. The changes enhance maintainability without altering the core test logic. All modifications adhere to the provided coding guidelines and Jest best practices.
To ensure that the new theme definitions are correctly implemented and no unintended changes were made, please run the following verification script:
This script will help confirm that the new theme definitions are correctly implemented and used consistently across the codebase.
✅ Verification successful
The previous verification scripts encountered issues because
rgdoes not recognizetypescriptas a predefined file type. To proceed, please run the following updated verification scripts that explicitly target.tsand.tsxfiles:Please execute these scripts to gather the necessary information for verifying the review comment.
Verification Successful: Theme Definitions Properly Implemented
The new theme definitions
MONACO_LIGHT_THEME_DEFINITIONandMONACO_DARK_THEME_DEFINITIONare correctly exported from their respective files and properly utilized in the test file. Additionally, no instances of the old theme IDs are found elsewhere in the codebase.
- Verified exports in
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts- Verified exports in
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-dark.theme.ts- Confirmed no other files use
MonacoEditorService.(LIGHT|DARK)_THEME_IDoutside the test file🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of new theme definitions and their usage in tests. # Test 1: Check if the new theme definitions are exported correctly rg --type typescript "export const MONACO_(LIGHT|DARK)_THEME_DEFINITION" app/shared/monaco-editor/model/themes/ # Test 2: Verify that the test file is using the new theme definitions correctly rg --type typescript "MONACO_(LIGHT|DARK)_THEME_DEFINITION.id" src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts # Test 3: Ensure no other files are still using the old theme IDs rg --type typescript "MonacoEditorService\.(LIGHT|DARK)_THEME_ID" -g '!src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts'Length of output: 529
Script:
#!/bin/bash # Description: Verify the implementation of new theme definitions and their usage in tests. # Test 1: Check if the new theme definitions are exported correctly rg "export const MONACO_(LIGHT|DARK)_THEME_DEFINITION" app/shared/monaco-editor/model/themes/ -g "*.ts" -g "*.tsx" # Test 2: Verify that the test file is using the new theme definitions correctly rg "MONACO_(LIGHT|DARK)_THEME_DEFINITION.id" src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts -g "*.ts" -g "*.tsx" # Test 3: Ensure no other files are still using the old theme IDs rg "MonacoEditorService\.(LIGHT|DARK)_THEME_ID" --glob '!src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts' -g "*.ts" -g "*.tsx"Length of output: 790
Script:
#!/bin/bash # Description: Locate the export statements for MONACO_LIGHT_THEME_DEFINITION and MONACO_DARK_THEME_DEFINITION. # Search for export of MONACO_LIGHT_THEME_DEFINITION rg "export const MONACO_LIGHT_THEME_DEFINITION" --glob "*.ts" --glob "*.tsx" # Search for export of MONACO_DARK_THEME_DEFINITION rg "export const MONACO_DARK_THEME_DEFINITION" --glob "*.ts" --glob "*.tsx"Length of output: 446
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (6)
6-8: Import New Theme Definitions and ModelThe import statements for
MONACO_LIGHT_THEME_DEFINITION,MonacoEditorTheme, andMONACO_DARK_THEME_DEFINITIONare correctly added and essential for implementing custom theming functionality.
20-21: Define Private Theme PropertiesThe declaration of
lightThemeanddarkThemeas private properties follows the coding guidelines for property naming conventions (camelCase) and encapsulation.
24-25: Initialize Themes and Languages in ConstructorInvoking
registerCustomThemes()andregisterCustomMarkdownLanguage()in the constructor ensures that the editor is properly configured upon service instantiation.
32-37: Register Custom ThemesThe
registerCustomThemesmethod correctly initializes and registers the custom light and dark themes, adhering to best practices by encapsulating theme setup within a dedicated method.
51-51: Apply Theme Based on Current ThemeThe
applyThememethod correctly sets the editor theme based on the current Artemis theme using a ternary operator, ensuring synchronization between the application and editor themes.
97-100: Customize Diff Editor AppearanceThe addition of the
guidesoption withindentation: falseand settingrenderLineHighlightto'none'effectively customizes the diff editor to match the desired styling, enhancing user experience.
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-theme-definition.interface.ts
Show resolved
Hide resolved
...in/webapp/app/shared/monaco-editor/model/themes/language-token-style-definition.interface.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-light.theme.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/themes/editor-colors.interface.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/monaco-editor/model/themes/monaco-editor-theme.model.ts
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
az108
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
raffifasaro
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
General: Add custom themes for the Monaco editorProgramming exercises: Add custom themes for the Monaco editor
krusche
added
ready to merge
maintainer-approved
Checklist
General
Client
Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.(see Allow CSS variables in themes microsoft/monaco-editor#2427)Changes affecting Programming Exercises
Motivation and Context
We would like to have more control over the styling of the Monaco editor. Currently, we only use the default themes.
Description
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Before
After
Summary by CodeRabbit
New Features
Bug Fixes
Tests