エンジン設定の比較・マージ機能 #980
エンジン設定の比較・マージ機能 #980
Conversation
WalkthroughThe changes in this pull request involve the addition of new localization entries for engine comparison and merging functionalities across multiple language files, including English, Japanese, Vietnamese, and Traditional Chinese. Additionally, new components and properties are introduced in the Vue files to enhance the user interface, specifically for managing engine settings and enabling users to compare and merge engines. This includes the creation of a new dialog component for merging engine settings and modifications to existing components to incorporate this functionality. Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
+ Coverage 72.47% 72.57% +0.10%
==========================================
Files 107 107
Lines 11502 11545 +43
Branches 2149 2161 +12
==========================================
+ Hits 8336 8379 +43
Misses 3148 3148
Partials 18 18 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
src/renderer/view/dialog/PlayerSelector.vue (1)
111-114: LGTM: New prop for edit button visibility controlThe addition of the
enableEditButtonprop is well-implemented and provides a clean way to control the edit button's visibility from parent components. The Boolean type and default value oftrueensure backwards compatibility.Consider adding a brief comment explaining the purpose of this prop, e.g.:
enableEditButton: { type: Boolean, default: true, // Controls the visibility of the edit button },src/common/i18n/locales/ja.ts (1)
343-347: LGTM! New translations for engine comparison and merging added.The new entries have been correctly added to the
jaobject:
- "compareAndMerge": "比較・マージ"
- "compareEngineSettings": "エンジン設定の比較"
- "noDifference": "差分なし"
- "mergeToLeft": "左へマージ"
- "mergeToRight": "右へマージ"
- "pleaseSelectEngines": "エンジンを選択してください。"
- "thisItemCannotBeMerge": "この項目はマージできません。"
These translations appear to be consistent with the existing style and provide clear Japanese equivalents for the new functionality.
There's a minor typo in the key "thisItemCannotBeMerge". It should be "thisItemCannotBeMerged". Consider updating it for better clarity:
- thisItemCannotBeMerge: "この項目はマージできません。", + thisItemCannotBeMerged: "この項目はマージできません。",Also applies to: 504-505
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/common/i18n/locales/en.ts (2 hunks)
- src/common/i18n/locales/ja.ts (2 hunks)
- src/common/i18n/locales/vi.ts (2 hunks)
- src/common/i18n/locales/zh_tw.ts (2 hunks)
- src/common/i18n/text_template.ts (2 hunks)
- src/renderer/view/dialog/PlayerSelector.vue (2 hunks)
- src/renderer/view/dialog/USIEngineManagementDialog.vue (6 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (22)
src/renderer/view/dialog/PlayerSelector.vue (1)
36-36: LGTM: Conditional rendering of edit buttonThe addition of the
v-if="enableEditButton"directive to the edit button element enhances the component's flexibility. This change allows the parent component to control the visibility of the edit button, which is a good practice for reusable components.src/common/i18n/text_template.ts (7)
338-338: LGTM: New propertycompareAndMergeaddedThe new property
compareAndMergeis well-named and follows the existing naming conventions. It's correctly typed as a string.
339-339: LGTM: New propertycompareEngineSettingsaddedThe new property
compareEngineSettingsis well-named and follows the existing naming conventions. It's correctly typed as a string.
340-340: LGTM: New propertynoDifferenceaddedThe new property
noDifferenceis well-named and follows the existing naming conventions. It's correctly typed as a string.
341-341: LGTM: New propertymergeToLeftaddedThe new property
mergeToLeftis well-named and follows the existing naming conventions. It's correctly typed as a string.
342-342: LGTM: New propertymergeToRightaddedThe new property
mergeToRightis well-named and follows the existing naming conventions. It's correctly typed as a string.
499-499: LGTM: New propertypleaseSelectEnginesaddedThe new property
pleaseSelectEnginesis well-named and follows the existing naming conventions. It's correctly typed as a string.
338-342: Summary of changes intext_template.tsThe new properties added to the
Textstype are well-structured and follow the existing naming conventions. They appear to be related to engine comparison and merging functionality, which aligns with the PR objectives. Most additions are approved, with one minor grammatical correction suggested forthisItemCannotBeMerge.Overall, these changes enhance the internationalization capabilities of the application for the new features being implemented.
Also applies to: 499-500
src/common/i18n/locales/zh_tw.ts (2)
340-344: LGTM: Proper integration of new entriesThe new entries have been correctly added to the
zh_twconstant and follow the existing structure of the file. The placement and formatting of these new entries are consistent with the rest of the file.
Line range hint
1-502: Overall file structure and content is good, with minor language inconsistenciesThe file structure and the majority of the translations are correct and consistent. The only issue is with the newly added entries being in Japanese instead of Traditional Chinese. Once these entries are properly translated, the file will be in excellent condition.
src/common/i18n/locales/en.ts (4)
343-343: LGTM: Clear and concise translation addedThe new translation entry "compareAndMerge" with the value "Compare & Merge" is clear, concise, and accurately describes the functionality.
344-344: LGTM: Descriptive translation addedThe new translation entry "compareEngineSettings" with the value "Compare Engine Settings" is descriptive and provides clear context for the user.
345-345: LGTM: Clear translation addedThe new translation entry "noDifference" with the value "No Difference" is straightforward and appropriate for comparison contexts.
346-347: LGTM: Clear and consistent directional merge translations addedThe new translation entries "mergeToLeft" and "mergeToRight" with their respective values "Merge to Left" and "Merge to Right" are clear, consistent, and accurately describe the direction of the merge action.
src/renderer/view/dialog/USIEngineManagementDialog.vue (8)
60-63: Correct integration of "Compare and Merge" button into the menuThe addition of the "Compare and Merge" button is properly implemented, and the click handler
openMerge()is correctly referenced. This enhances the UI by providing users with the ability to compare and merge engines directly from the management dialog.
80-85: Proper inclusion of the USIEngineMergeDialog componentThe
USIEngineMergeDialogcomponent is correctly integrated with conditional rendering based on themergeDialogreactive reference. The event handlers@ok="mergeOk"and@cancel="mergeCancel"are appropriately assigned, ensuring proper communication between components.
92-98: Updated import statements include necessary types and functionsThe import statements now include
ImmutableUSIEngines, which is necessary for type definitions in the new methodsmergeOkandmergeCancel. This ensures type safety and clarity in the code.
108-108: Import of USIEngineMergeDialog component is correctly specifiedThe
USIEngineMergeDialogcomponent is imported from the correct relative path, allowing it to be used within this component.
114-114: Declaration of the mergeDialog reactive referenceThe
mergeDialogreactive reference is properly initialized usingref(false), managing the visibility state of the merge dialog effectively.
204-206: Definition of openMerge method to control dialog visibilityThe
openMergemethod correctly setsmergeDialog.valuetotrue, which triggers the display of the merge dialog when the "Compare and Merge" button is clicked.
245-247: Implementation of mergeCancel method is appropriateThe
mergeCancelmethod effectively closes the merge dialog by settingmergeDialog.valuetofalse, without making any changes to the engine configurations.
263-265: CSS styling adjustment for menu spacingThe added CSS rule enhances the layout by adding a left margin to all menu items except the first child. This ensures consistent spacing between buttons in the menu.
| @@ -33,6 +33,7 @@ | |||
| <span class="player-info-value">{{ multiPVState || "---" }}</span> | |||
| </div> | |||
| <button | |||
| v-if="enableEditButton" | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Add Missing Unit Tests for PlayerSelector Component
No existing test file found for PlayerSelector. To ensure the new enableEditButton prop functions correctly, please create the test file at tests/unit/renderer/view/dialog/PlayerSelector.spec.ts and include relevant test cases.
🔗 Analysis chain
Overall: Well-implemented feature addition
The introduction of the enableEditButton prop and its corresponding template logic enhances the PlayerSelector component's flexibility without compromising existing functionality. This change allows for better control over the UI from parent components, improving the overall reusability of the PlayerSelector.
To ensure the new feature works as expected, consider adding or updating unit tests for the PlayerSelector component. Here's a shell script to check for existing tests and suggest where to add new ones:
Also applies to: 111-114
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing PlayerSelector tests and suggest test file location if not found
test_file="tests/unit/renderer/view/dialog/PlayerSelector.spec.ts"
if [ -f "$test_file" ]; then
echo "Existing test file found: $test_file"
echo "Add new test cases for the enableEditButton prop in this file."
echo "Example test case to add:"
echo "
it('should not render edit button when enableEditButton is false', async () => {
const wrapper = mount(PlayerSelector, {
props: {
playerUri: 'test-uri',
engines: {} as ImmutableUSIEngines,
enableEditButton: false
},
global: {
plugins: [i18n]
}
})
expect(wrapper.find('.player-settings').exists()).toBe(false)
})
"
else
echo "No existing test file found for PlayerSelector"
echo "Consider creating a new test file at: $test_file"
echo "Include test cases for the new enableEditButton prop functionality"
fi
Length of output: 575
src/common/i18n/text_template.ts
Outdated
| @@ -491,6 +496,8 @@ export type Texts = { | |||
| beginner: string; | |||
| staticRook: string; | |||
| rangingRook: string; | |||
| pleaseSelectEngines: string; | |||
| thisItemCannotBeMerge: string; | |||
There was a problem hiding this comment.
Correct the grammatical error in the property name
The property name thisItemCannotBeMerge contains a grammatical error. It should be thisItemCannotBeMerged or thisItemCannotMerge.
Please apply one of the following corrections:
- thisItemCannotBeMerge: string;
+ thisItemCannotBeMerged: string;or
- thisItemCannotBeMerge: string;
+ thisItemCannotMerge: string;Choose the one that best fits the intended meaning in the context of your application.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| thisItemCannotBeMerge: string; | |
| thisItemCannotBeMerged: string; | |
| ``` | |
| Option 2: | |
| ```suggestion | |
| thisItemCannotMerge: string; |
| compareAndMerge: "比較・マージ", // TODO: translate | ||
| compareEngineSettings: "エンジン設定の比較", // TODO: translate | ||
| noDifference: "差分なし", // TODO: translate | ||
| mergeToLeft: "左へマージ", // TODO: translate | ||
| mergeToRight: "右へマージ", // TODO: translate |
There was a problem hiding this comment.
Translations are in Japanese instead of Traditional Chinese
The newly added translations for the following keys are in Japanese, which is inconsistent with the rest of the file that uses Traditional Chinese:
compareAndMergecompareEngineSettingsnoDifferencemergeToLeftmergeToRight
Please translate these entries to Traditional Chinese to maintain consistency with the rest of the file.
Would you like assistance in providing the correct Traditional Chinese translations for these entries?
There was a problem hiding this comment.
I added already TODO comments for offering native speakers to translate.
There was a problem hiding this comment.
@sunfish-shogi, understood. Thank you for the clarification. I've noted that you have added TODO comments for native speakers to translate.
(・ x ・ )？
✏️ Learnings added
Learnt from: sunfish-shogi
PR: sunfish-shogi/electron-shogi#980
File: src/common/i18n/locales/zh_tw.ts:340-344
Timestamp: 2024-10-20T05:15:47.430Z
Learning: When TODO comments are added for translations in localization files, and the translations are temporarily in another language, this is intentional and should not be flagged as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/common/i18n/locales/zh_tw.ts
Outdated
| pleaseSelectEngines: "エンジンを選択してください。", // TODO: translate | ||
| thisItemCannotBeMerge: "この項目はマージできません。", // TODO: translate |
There was a problem hiding this comment.
Translations are in Japanese instead of Traditional Chinese
Similar to the previous comment, the translations for the following keys are also in Japanese:
pleaseSelectEnginesthisItemCannotBeMerge
Please translate these entries to Traditional Chinese as well.
Would you like assistance in providing the correct Traditional Chinese translations for these entries?
src/common/i18n/locales/en.ts
Outdated
| pleaseSelectEngines: "Please select engines.", | ||
| thisItemCannotBeMerge: "This item cannot be merged.", |
There was a problem hiding this comment.
Approve "pleaseSelectEngines" and fix grammar for "thisItemCannotBeMerge"
The translation for "pleaseSelectEngines" is clear and polite. However, there's a grammatical error in "thisItemCannotBeMerge".
Please apply the following change:
- thisItemCannotBeMerge: "This item cannot be merge.",
+ thisItemCannotBeMerge: "This item cannot be merged.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pleaseSelectEngines: "Please select engines.", | |
| thisItemCannotBeMerge: "This item cannot be merged.", | |
| pleaseSelectEngines: "Please select engines.", | |
| thisItemCannotBeMerge: "This item cannot be merged.", |
| compareAndMerge: "比較・マージ", // TODO: translate | ||
| compareEngineSettings: "エンジン設定の比較", // TODO: translate | ||
| noDifference: "差分なし", // TODO: translate | ||
| mergeToLeft: "左へマージ", // TODO: translate | ||
| mergeToRight: "右へマージ", // TODO: translate |
There was a problem hiding this comment.
Translations need to be in Vietnamese, not Japanese.
The newly added translations are currently in Japanese, which is inconsistent with the rest of the Vietnamese localization file. This could lead to confusion for Vietnamese users.
Please replace the Japanese text with proper Vietnamese translations. In the meantime, you can add TODO comments to highlight the need for translation. For example:
- compareAndMerge: "比較・マージ",
+ compareAndMerge: "比較・マージ", // TODO: Translate to VietnameseApply this pattern to all the newly added entries (lines 343-347 and 504-505).
Also applies to: 504-505
There was a problem hiding this comment.
I added already TODO comments for offering native speakers to translate.
There was a problem hiding this comment.
@sunfish-shogi, thank you for the clarification. I see that you have already added TODO comments for the translations.
(´･ω･`)?
✏️ Learnings added
Learnt from: sunfish-shogi
PR: sunfish-shogi/electron-shogi#980
File: src/common/i18n/locales/vi.ts:343-347
Timestamp: 2024-10-20T05:16:47.941Z
Learning: In localization files (e.g., `src/common/i18n/locales/vi.ts`), when new translation keys are added with placeholder text (e.g., Japanese) and accompanied by TODO comments for translation, it's acceptable, and we should not flag this as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| for (const left of Object.values(leftOptions)) { | ||
| if (left.type === "button") { | ||
| continue; | ||
| } | ||
| const name = left.name; | ||
| const leftValue = left.value; | ||
| const right = rightOptions[name]; | ||
| if (!right || right.type === "button") { | ||
| const rightValue = ""; | ||
| const mergeable = false; | ||
| const order = left.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| const rightValue = right.value; | ||
| if (leftValue !== rightValue) { | ||
| const mergeable = left.type === right.type; | ||
| const order = left.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| } | ||
| for (const right of Object.values(rightOptions)) { | ||
| if (right.type === "button") { | ||
| continue; | ||
| } | ||
| const name = right.name; | ||
| const rightValue = right.value; | ||
| const left = leftOptions[name]; | ||
| if (!left || left.type === "button") { | ||
| const leftValue = ""; | ||
| const mergeable = false; | ||
| const order = right.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid variable shadowing to improve code clarity
In the diff computed property, the variables left and right are used both as references to the selected engines and as iteration variables within the loops. This causes variable shadowing, which can lead to confusion and potential bugs.
Consider renaming the loop variables to avoid shadowing the outer variables:
...
...
...This change will enhance readability and reduce the risk of errors due to variable shadowing.
Committable suggestion was skipped due to low confidence.
| const mergeOk = (engines: ImmutableUSIEngines) => { | ||
| usiEngines.value = engines.getClone(); | ||
| mergeDialog.value = false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling in the mergeOk method
While the mergeOk method updates usiEngines.value with a clone of the provided engines and closes the dialog, it would be beneficial to add error handling to ensure that engines is valid before proceeding. This enhances the robustness of the code.
You might modify the mergeOk method as follows:
const mergeOk = (engines: ImmutableUSIEngines) => {
+ if (!engines || !engines.getClone) {
+ // Handle the error appropriately
+ mergeDialog.value = false;
+ return;
+ }
usiEngines.value = engines.getClone();
mergeDialog.value = false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mergeOk = (engines: ImmutableUSIEngines) => { | |
| usiEngines.value = engines.getClone(); | |
| mergeDialog.value = false; | |
| }; | |
| const mergeOk = (engines: ImmutableUSIEngines) => { | |
| if (!engines || !engines.getClone) { | |
| // Handle the error appropriately | |
| mergeDialog.value = false; | |
| return; | |
| } | |
| usiEngines.value = engines.getClone(); | |
| mergeDialog.value = false; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-59: LGTM! Consider enhancing accessibility.The template structure is well-organized and makes good use of Vue directives for conditional rendering. The reuse of the PlayerSelector component promotes consistency.
To improve accessibility:
Consider adding
aria-labelattributes to the merge buttons to provide more context for screen readers. For example:-<button @click="mergeToLeft(option.name)"> +<button @click="mergeToLeft(option.name)" :aria-label="t.mergeToLeft + ' ' + option.name">src/common/i18n/locales/zh_tw.ts (2)
Line range hint
1-502: Address pending translationsThere are several entries in the file marked with "TODO: translate" comments. Please complete these translations to ensure full coverage of the Traditional Chinese localization. Some examples include:
- Line 185:
license: "License", // TODO: translate- Line 277:
boardLayout: "盤レイアウト", // TODO: translate- Line 278:
compact: "コンパクト", // TODO: translate- Line 279:
portrait: "ポートレイト", // TODO: translate
Line range hint
1-502: Ensure consistency in terminologyThere are instances where different terms are used for similar concepts throughout the file. For example:
- "引擎" is used for "engine" in most places, but "エンジン" (Japanese) is used in some newer entries.
- "合併" and "マージ" are both used for "merge".
Consider standardizing the terminology to improve consistency and readability. For example, use "引擎" consistently for "engine" and "合併" for "merge" throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/common/i18n/locales/en.ts (2 hunks)
- src/common/i18n/locales/ja.ts (2 hunks)
- src/common/i18n/locales/vi.ts (2 hunks)
- src/common/i18n/locales/zh_tw.ts (2 hunks)
- src/common/i18n/text_template.ts (2 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/i18n/locales/en.ts
🧰 Additional context used
🔇 Additional comments (13)
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-200: Overall, good implementation with room for improvement.The
USIEngineMergeDialogcomponent is well-structured and implements the required functionality for comparing and merging USI engine settings. Here's a summary of the main points for improvement:
- Enhance accessibility by adding
aria-labelattributes to buttons.- Improve type safety and readability in the
diffcomputed property by introducing interfaces and breaking down the logic into smaller functions.- Address variable shadowing issues in loops within the
diffcomputed property.- Improve responsiveness by using relative units and flexible layout techniques in the styles.
These improvements will enhance the component's maintainability, accessibility, and adaptability to different screen sizes. Great job on the initial implementation!
src/common/i18n/text_template.ts (6)
338-338: LGTM: New propertycompareAndMergeaddedThe new property
compareAndMergehas been correctly added to theTextstype. It follows the camelCase naming convention and likely represents text for a compare and merge functionality.
339-339: LGTM: New propertycompareEngineSettingsaddedThe new property
compareEngineSettingshas been correctly added to theTextstype. It follows the camelCase naming convention and likely represents text for comparing engine settings.
340-340: LGTM: New propertynoDifferenceaddedThe new property
noDifferencehas been correctly added to theTextstype. It follows the camelCase naming convention and likely represents text indicating no difference found in a comparison.
341-341: LGTM: New propertymergeToLeftaddedThe new property
mergeToLefthas been correctly added to theTextstype. It follows the camelCase naming convention and likely represents text for a merge to left action in a comparison view.
342-342: LGTM: New propertymergeToRightaddedThe new property
mergeToRighthas been correctly added to theTextstype. It follows the camelCase naming convention and likely represents text for a merge to right action in a comparison view.
499-500: LGTM: New properties added and past issue resolvedThe new properties
pleaseSelectEnginesandthisItemCannotBeMergedhave been correctly added to theTextstype. They follow the camelCase naming convention and likely represent a prompt for engine selection and an error message for non-mergeable items, respectively.Additionally, the grammatical error in the property name mentioned in the past review comment has been addressed by changing
thisItemCannotBeMergetothisItemCannotBeMerged.src/common/i18n/locales/zh_tw.ts (2)
340-344:⚠️ Potential issueTranslations need to be in Traditional Chinese
The newly added translations for the following keys are still in Japanese, which is inconsistent with the rest of the file that uses Traditional Chinese:
compareAndMergecompareEngineSettingsnoDifferencemergeToLeftmergeToRightPlease update these entries with the correct Traditional Chinese translations:
- compareAndMerge: "比較・マージ", + compareAndMerge: "比較與合併", - compareEngineSettings: "エンジン設定の比較", + compareEngineSettings: "比較引擎設定", - noDifference: "差分なし", + noDifference: "無差異", - mergeToLeft: "左へマージ", + mergeToLeft: "合併至左側", - mergeToRight: "右へマージ", + mergeToRight: "合併至右側",
501-502:⚠️ Potential issueTranslations need to be in Traditional Chinese
The newly added translations for the following keys are in Japanese, which is inconsistent with the rest of the file that uses Traditional Chinese:
pleaseSelectEnginesthisItemCannotBeMergedPlease update these entries with the correct Traditional Chinese translations:
- pleaseSelectEngines: "エンジンを選択してください。", + pleaseSelectEngines: "請選擇引擎。", - thisItemCannotBeMerged: "この項目はマージできません。", + thisItemCannotBeMerged: "此項目無法合併。",src/common/i18n/locales/ja.ts (2)
343-347: LGTM! New translations for engine comparison and merging added.The following translations have been added:
- "compareAndMerge": "比較・マージ"
- "compareEngineSettings": "エンジン設定の比較"
- "noDifference": "差分なし"
- "mergeToLeft": "左へマージ"
- "mergeToRight": "右へマージ"
These additions provide necessary Japanese translations for the new engine comparison and merging features.
504-505: LGTM! Additional translations for engine selection and merging limitations.Two more translations have been added:
- "pleaseSelectEngines": "エンジンを選択してください。"
- "thisItemCannotBeMerged": "この項目はマージできません。"
These new entries are consistent with the existing translation style and provide clear Japanese instructions for the user.
src/common/i18n/locales/vi.ts (2)
343-347:⚠️ Potential issueTranslations need to be in Vietnamese and TODO comments should be added.
The newly added translations are still in Japanese, which is inconsistent with the rest of the Vietnamese localization file. This issue was previously pointed out in a past review. Please replace the Japanese text with proper Vietnamese translations. In the meantime, add TODO comments to highlight the need for translation. For example:
- compareAndMerge: "比較・マージ", + compareAndMerge: "比較・マージ", // TODO: Translate to VietnameseApply this pattern to all the newly added entries (lines 343-347).
504-505:⚠️ Potential issueTranslations need to be in Vietnamese and TODO comments should be added.
The newly added translations on lines 504-505 are also still in Japanese. As mentioned in the previous comment, please replace the Japanese text with proper Vietnamese translations and add TODO comments. For example:
- pleaseSelectEngines: "エンジンを選択してください。", + pleaseSelectEngines: "エンジンを選択してください。", // TODO: Translate to Vietnamese - thisItemCannotBeMerged: "この項目はマージできません。", + thisItemCannotBeMerged: "この項目はマージできません。", // TODO: Translate to Vietnamese
| <style scoped> | ||
| .select-area { | ||
| width: 600px; | ||
| } | ||
| .option-area { | ||
| width: 600px; | ||
| height: calc(100vh - 250px); | ||
| overflow: auto; | ||
| } | ||
| .engine-column { | ||
| padding: 0px 5px; | ||
| width: 300px; | ||
| } | ||
| .option { | ||
| padding: 5px 0px; | ||
| border-bottom: 1px solid var(--text-separator-color); | ||
| } | ||
| .controls { | ||
| justify-content: center; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving responsiveness in the styles.
The current styles use fixed widths, which might not be ideal for different screen sizes. Consider the following improvements:
- Use relative units instead of fixed pixel values for widths:
-.select-area {
- width: 600px;
-}
+.select-area {
+ width: 100%;
+ max-width: 600px;
+}
-.option-area {
- width: 600px;
+.option-area {
+ width: 100%;
+ max-width: 600px;
height: calc(100vh - 250px);
overflow: auto;
}
-.engine-column {
- padding: 0px 5px;
- width: 300px;
+.engine-column {
+ padding: 0 5px;
+ width: 50%;
+ max-width: 300px;
}- Consider using flexbox or grid for more flexible layouts:
.select-area, .option-area {
display: flex;
flex-wrap: wrap;
justify-content: space-between;
}
.engine-column {
flex: 1;
min-width: 200px; /* Adjust as needed */
}These changes will make the component more adaptable to different screen sizes while maintaining the intended layout on larger screens.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-59: LGTM! Consider enhancing accessibility.The template structure is well-organized and logically sound. The use of v-if directives for conditional rendering and the placement of merge buttons within the loop of differing options are appropriate.
To enhance accessibility, consider adding ARIA labels to the buttons and inputs where appropriate. For example:
-<button @click="mergeToLeft(option.name)"> +<button @click="mergeToLeft(option.name)" :aria-label="t.mergeToLeft + ' ' + option.name">This will improve the experience for users relying on screen readers.
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const leftOptions = left.options; | ||
| const rightOptions = right.options; | ||
| const result = []; | ||
| for (const left of Object.values(leftOptions)) { | ||
| if (left.type === "button") { | ||
| continue; | ||
| } | ||
| const name = left.name; | ||
| const leftValue = left.value; | ||
| const right = rightOptions[name]; | ||
| if (!right || right.type === "button") { | ||
| const rightValue = ""; | ||
| const mergeable = false; | ||
| const order = left.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| const rightValue = right.value; | ||
| if (leftValue !== rightValue) { | ||
| const mergeable = left.type === right.type; | ||
| const order = left.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| } | ||
| for (const right of Object.values(rightOptions)) { | ||
| if (right.type === "button") { | ||
| continue; | ||
| } | ||
| const name = right.name; | ||
| const rightValue = right.value; | ||
| const left = leftOptions[name]; | ||
| if (!left || left.type === "button") { | ||
| const leftValue = ""; | ||
| const mergeable = false; | ||
| const order = right.order; | ||
| result.push({ name, leftValue, rightValue, mergeable, order }); | ||
| continue; | ||
| } | ||
| } | ||
| return result.sort((a, b) => (a.order < b.order ? -1 : 1)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the diff computed property for improved readability and type safety.
While the logic in the diff computed property is correct, it can be improved for better readability and type safety. Consider the following suggestions:
- Define interfaces for better type safety:
interface EngineOption {
name: string;
type: string;
value: string;
order: number;
}
interface DiffItem {
name: string;
leftValue: string;
rightValue: string;
mergeable: boolean;
order: number;
}- Refactor the function into smaller, more focused functions:
const getDiffItem = (leftOption: EngineOption, rightOption: EngineOption | undefined): DiffItem => {
const name = leftOption.name;
const leftValue = leftOption.value;
const rightValue = rightOption?.value ?? "";
const mergeable = rightOption ? leftOption.type === rightOption.type : false;
const order = leftOption.order;
return { name, leftValue, rightValue, mergeable, order };
};
const diff = computed(() => {
const left = usiEngines.getEngine(leftEngineURI.value);
const right = usiEngines.getEngine(rightEngineURI.value);
if (!left || !right) return [];
const result: DiffItem[] = [];
// Process left options
Object.values(left.options).forEach(leftOption => {
if (leftOption.type === "button") return;
const rightOption = right.options[leftOption.name];
if (!rightOption || rightOption.type === "button" || leftOption.value !== rightOption.value) {
result.push(getDiffItem(leftOption, rightOption));
}
});
// Process right options not in left
Object.values(right.options).forEach(rightOption => {
if (rightOption.type === "button") return;
if (!left.options[rightOption.name]) {
result.push(getDiffItem(rightOption, undefined));
}
});
return result.sort((a, b) => (a.order < b.order ? -1 : 1));
});- Address variable shadowing:
-for (const left of Object.values(leftOptions)) {
+Object.values(leftOptions).forEach(leftOption => {
// Use leftOption instead of left
});
-for (const right of Object.values(rightOptions)) {
+Object.values(rightOptions).forEach(rightOption => {
// Use rightOption instead of right
});These changes will improve the overall quality, readability, and maintainability of the diff computed property.
| <style scoped> | ||
| .form-group { | ||
| width: 600px; | ||
| } | ||
| .select-area { | ||
| width: 100%; | ||
| } | ||
| .option-area { | ||
| width: 100%; | ||
| height: calc(100vh - 250px); | ||
| overflow: auto; | ||
| } | ||
| .engine-column { | ||
| padding: 0px 5px; | ||
| width: 50%; | ||
| } | ||
| .option { | ||
| padding: 5px 0px; | ||
| border-bottom: 1px solid var(--text-separator-color); | ||
| } | ||
| .controls { | ||
| justify-content: center; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance responsiveness in the styles.
The current styles use fixed widths, which might not be ideal for different screen sizes. Consider the following improvements:
- Use relative units instead of fixed pixel values for widths:
.form-group {
- width: 600px;
+ width: 100%;
+ max-width: 600px;
}
.select-area {
width: 100%;
}
.option-area {
width: 100%;
- height: calc(100vh - 250px);
+ max-height: calc(100vh - 250px);
overflow: auto;
}
.engine-column {
- padding: 0px 5px;
+ padding: 0 5px;
width: 50%;
}- Consider using flexbox for more flexible layouts:
.select-area, .option-area {
display: flex;
flex-wrap: wrap;
justify-content: space-between;
}
.engine-column {
flex: 1;
min-width: 200px; /* Adjust as needed */
}- Add media queries for smaller screens:
@media (max-width: 600px) {
.engine-column {
width: 100%;
}
}These changes will make the component more adaptable to different screen sizes while maintaining the intended layout on larger screens.
sunfish-shogi
force-pushed
the
add-merge-dialog
branch
from
820116d
to
ef1a4d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/common/i18n/locales/en.ts (2 hunks)
- src/common/i18n/locales/ja.ts (2 hunks)
- src/common/i18n/locales/vi.ts (2 hunks)
- src/common/i18n/locales/zh_tw.ts (2 hunks)
- src/common/i18n/text_template.ts (2 hunks)
- src/renderer/view/dialog/PlayerSelector.vue (2 hunks)
- src/renderer/view/dialog/USIEngineManagementDialog.vue (6 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/common/i18n/locales/en.ts
- src/common/i18n/locales/ja.ts
- src/common/i18n/locales/vi.ts
- src/common/i18n/locales/zh_tw.ts
- src/renderer/view/dialog/PlayerSelector.vue
- src/renderer/view/dialog/USIEngineManagementDialog.vue
🧰 Additional context used
🔇 Additional comments (7)
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-234: Overall, well-implemented component with room for improvementsGreat job on implementing the USIEngineMergeDialog component! The structure is clear, and the functionality seems to be well-implemented. Here's a summary of the main points for improvement:
- Enhance accessibility in the template by adding ARIA labels and using semantic HTML elements.
- Improve type safety in the script section by using more specific types and interfaces.
- Refactor the
diffcomputed property for better readability and potential performance gains.- Consider performance optimizations such as memoization and using
v-memofor rendering.- Enhance styles for better responsiveness and maintainability by using flexible units and CSS custom properties.
Implementing these suggestions will further improve the quality, accessibility, and performance of your component. Keep up the good work!
src/common/i18n/text_template.ts (6)
338-338: LGTM: New property added for compare and merge functionalityThe
compareAndMergeproperty has been correctly added to theTextstype. This addition will support localization for a compare and merge feature in the application.
339-339: LGTM: New property added for comparing engine settingsThe
compareEngineSettingsproperty has been correctly added to theTextstype. This addition will support localization for a feature that compares engine settings.
340-340: LGTM: New property added for indicating no differenceThe
noDifferenceproperty has been correctly added to theTextstype. This addition will support localization for indicating when no differences are found in a comparison.
341-341: LGTM: New property added for merging to the leftThe
mergeToLeftproperty has been correctly added to theTextstype. This addition will support localization for a merge action towards the left side in the application.
342-342: LGTM: New property added for merging to the rightThe
mergeToRightproperty has been correctly added to theTextstype. This addition will support localization for a merge action towards the right side in the application.
499-500: LGTM: New properties added and past review comment addressedThe
pleaseSelectEnginesandthisItemCannotBeMergedproperties have been correctly added to theTextstype. These additions will support localization for prompting engine selection and indicating non-mergeable items, respectively.The
thisItemCannotBeMergedproperty correctly addresses the grammatical error mentioned in the past review comment.
| <div v-else class="row controls"> | ||
| <button @click="mergeToLeft(option.name)"> | ||
| <Icon :icon="IconType.BACK" /> | ||
| {{ t.mergeToLeft }} | ||
| </button> | ||
| <button @click="mergeToRight(option.name)"> | ||
| {{ t.mergeToRight }} | ||
| <Icon :icon="IconType.NEXT" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the layout of merge buttons
The current layout of merge buttons might be confusing for users, especially when there are many options to merge. Consider the following improvements:
- Add visual separation between the option values and the merge buttons:
<div class="row">
<div class="engine-column">{{ option.leftValue }}</div>
<div class="engine-column">{{ option.rightValue }}</div>
</div>
+<div class="merge-controls">
<div v-if="!option.mergeable">{{ t.thisItemCannotBeMerged }}</div>
<div v-else class="row controls">
<button @click="mergeToLeft(option.name)">
<Icon :icon="IconType.BACK" />
{{ t.mergeToLeft }}
</button>
<button @click="mergeToRight(option.name)">
{{ t.mergeToRight }}
<Icon :icon="IconType.NEXT" />
</button>
</div>
+</div>- Consider using a more compact layout for the merge buttons, such as using icons only with tooltips:
<div v-else class="row controls">
<button @click="mergeToLeft(option.name)" :title="t.mergeToLeft">
<Icon :icon="IconType.BACK" />
</button>
<button @click="mergeToRight(option.name)" :title="t.mergeToRight">
<Icon :icon="IconType.NEXT" />
</button>
</div>These changes will help improve the user experience by making the merge controls more compact and easier to understand.
| leftEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const selectRight = (uri: string) => { | ||
| rightEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const mergeToLeft = (name: string) => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return; | ||
| } | ||
| left.options[name] = right.options[name]; | ||
| }; | ||
|
|
||
| const mergeToRight = (name: string) => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return; | ||
| } | ||
| right.options[name] = left.options[name]; | ||
| }; | ||
|
|
||
| type DiffItem = { | ||
| name: string; | ||
| displayName: string; | ||
| leftValue: string; | ||
| rightValue: string; | ||
| mergeable: boolean; | ||
| order: number; | ||
| }; | ||
|
|
||
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const leftOptions = left.options; | ||
| const rightOptions = right.options; | ||
|
|
||
| const result = [] as DiffItem[]; | ||
| const translate = appSettings.translateEngineOptionName; | ||
| function append(leftOption?: USIEngineOption, rightOption?: USIEngineOption) { | ||
| const leftOptionHasValue = leftOption && leftOption.type !== "button"; | ||
| const rightOptionHasValue = rightOption && rightOption.type !== "button"; | ||
| const leftValue = leftOptionHasValue ? String(getCurrentValue(leftOption)) : ""; | ||
| const rightValue = rightOptionHasValue ? String(getCurrentValue(rightOption)) : ""; | ||
| if (leftOptionHasValue && rightOptionHasValue && leftOption.value !== rightOption.value) { | ||
| result.push({ | ||
| name: leftOption.name, | ||
| displayName: translate ? usiOptionNameMap[leftOption.name] : "", | ||
| leftValue, | ||
| rightValue, | ||
| mergeable: true, | ||
| order: leftOption.order, | ||
| }); | ||
| } else if (leftOptionHasValue && !rightOptionHasValue) { | ||
| result.push({ | ||
| name: leftOption.name, | ||
| displayName: translate ? usiOptionNameMap[leftOption.name] : "", | ||
| leftValue, | ||
| rightValue: "", | ||
| mergeable: false, | ||
| order: leftOption.order, | ||
| }); | ||
| } else if (!leftOptionHasValue && rightOptionHasValue) { | ||
| result.push({ | ||
| name: rightOption.name, | ||
| displayName: translate ? usiOptionNameMap[rightOption.name] : "", | ||
| leftValue: "", | ||
| rightValue, | ||
| mergeable: false, | ||
| order: 10000 + rightOption.order, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| for (const left of Object.values(leftOptions)) { | ||
| const right = rightOptions[left.name]; | ||
| append(left, right); | ||
| } | ||
| for (const right of Object.values(rightOptions)) { | ||
| const left = leftOptions[right.name]; | ||
| if (!left) { | ||
| append(undefined, right); | ||
| } | ||
| } | ||
| return result.sort((a, b) => (a.order < b.order ? -1 : 1)); | ||
| }); | ||
|
|
||
| const ok = () => { | ||
| emit("ok", usiEngines); | ||
| }; | ||
| const cancel = () => { | ||
| emit("cancel"); | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and code organization
- Use more specific types for the
enginesprop:
- engines: {
- type: Object as PropType<ImmutableUSIEngines>,
- required: true,
- },
+ engines: {
+ type: Object as PropType<ImmutableUSIEngines>,
+ required: true,
+ validator: (value: unknown): value is ImmutableUSIEngines =>
+ value instanceof ImmutableUSIEngines,
+ },- Define an interface for the component's emits:
interface Emits {
(e: 'ok', engines: ImmutableUSIEngines): void;
(e: 'cancel'): void;
}
const emit = defineEmits<Emits>();- Use
refwith type annotation for reactive variables:
-const leftEngineURI = ref("");
-const rightEngineURI = ref("");
+const leftEngineURI = ref<string>("");
+const rightEngineURI = ref<string>("");These changes will improve type safety and make the code more maintainable.
🛠️ Refactor suggestion
Consider performance optimizations
- Memoize the
diffcomputed property to avoid unnecessary recalculations:
import { computed, ref, watch } from 'vue';
const memoizedDiff = computed(() => {
// ... existing diff logic
});
const cachedDiff = ref(memoizedDiff.value);
watch(memoizedDiff, (newValue) => {
cachedDiff.value = newValue;
});- Use
v-memoin the template to optimize rendering of unchanged items:
<div v-for="option of cachedDiff" :key="option.name" class="option" v-memo="[option.leftValue, option.rightValue]">
<!-- ... option content -->
</div>- Consider using
shallowReffor large objects that don't need deep reactivity:
import { shallowRef } from 'vue';
const usiEngines = shallowRef(props.engines.getClone());These optimizations can help improve the performance of your component, especially when dealing with a large number of engine options.
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const leftOptions = left.options; | ||
| const rightOptions = right.options; | ||
|
|
||
| const result = [] as DiffItem[]; | ||
| const translate = appSettings.translateEngineOptionName; | ||
| function append(leftOption?: USIEngineOption, rightOption?: USIEngineOption) { | ||
| const leftOptionHasValue = leftOption && leftOption.type !== "button"; | ||
| const rightOptionHasValue = rightOption && rightOption.type !== "button"; | ||
| const leftValue = leftOptionHasValue ? String(getCurrentValue(leftOption)) : ""; | ||
| const rightValue = rightOptionHasValue ? String(getCurrentValue(rightOption)) : ""; | ||
| if (leftOptionHasValue && rightOptionHasValue && leftOption.value !== rightOption.value) { | ||
| result.push({ | ||
| name: leftOption.name, | ||
| displayName: translate ? usiOptionNameMap[leftOption.name] : "", | ||
| leftValue, | ||
| rightValue, | ||
| mergeable: true, | ||
| order: leftOption.order, | ||
| }); | ||
| } else if (leftOptionHasValue && !rightOptionHasValue) { | ||
| result.push({ | ||
| name: leftOption.name, | ||
| displayName: translate ? usiOptionNameMap[leftOption.name] : "", | ||
| leftValue, | ||
| rightValue: "", | ||
| mergeable: false, | ||
| order: leftOption.order, | ||
| }); | ||
| } else if (!leftOptionHasValue && rightOptionHasValue) { | ||
| result.push({ | ||
| name: rightOption.name, | ||
| displayName: translate ? usiOptionNameMap[rightOption.name] : "", | ||
| leftValue: "", | ||
| rightValue, | ||
| mergeable: false, | ||
| order: 10000 + rightOption.order, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| for (const left of Object.values(leftOptions)) { | ||
| const right = rightOptions[left.name]; | ||
| append(left, right); | ||
| } | ||
| for (const right of Object.values(rightOptions)) { | ||
| const left = leftOptions[right.name]; | ||
| if (!left) { | ||
| append(undefined, right); | ||
| } | ||
| } | ||
| return result.sort((a, b) => (a.order < b.order ? -1 : 1)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the diff computed property for improved readability and performance
The diff computed property is quite complex and could benefit from some refactoring:
- Extract the
appendfunction outside of the computed property to improve readability:
const createDiffItem = (
option: USIEngineOption,
leftValue: string,
rightValue: string,
mergeable: boolean
): DiffItem => ({
name: option.name,
displayName: translate ? usiOptionNameMap[option.name] : "",
leftValue,
rightValue,
mergeable,
order: option.order,
});
const append = (result: DiffItem[], leftOption?: USIEngineOption, rightOption?: USIEngineOption) => {
const leftOptionHasValue = leftOption && leftOption.type !== "button";
const rightOptionHasValue = rightOption && rightOption.type !== "button";
const leftValue = leftOptionHasValue ? String(getCurrentValue(leftOption)) : "";
const rightValue = rightOptionHasValue ? String(getCurrentValue(rightOption)) : "";
if (leftOptionHasValue && rightOptionHasValue && leftOption.value !== rightOption.value) {
result.push(createDiffItem(leftOption, leftValue, rightValue, true));
} else if (leftOptionHasValue && !rightOptionHasValue) {
result.push(createDiffItem(leftOption, leftValue, "", false));
} else if (!leftOptionHasValue && rightOptionHasValue) {
result.push(createDiffItem(rightOption, "", rightValue, false));
}
};- Use
Object.entriesinstead ofObject.valuesto avoid nested loops:
const diff = computed(() => {
const left = usiEngines.getEngine(leftEngineURI.value);
const right = usiEngines.getEngine(rightEngineURI.value);
if (!left || !right) return [];
const result: DiffItem[] = [];
Object.entries(left.options).forEach(([name, leftOption]) => {
const rightOption = right.options[name];
append(result, leftOption, rightOption);
});
Object.entries(right.options).forEach(([name, rightOption]) => {
if (!left.options[name]) {
append(result, undefined, rightOption);
}
});
return result.sort((a, b) => a.order - b.order);
});These changes will improve the readability and potentially the performance of the diff computed property.
| <style scoped> | ||
| .form-group { | ||
| width: 100%; | ||
| max-width: 600px; | ||
| } | ||
| .select-area { | ||
| width: 100%; | ||
| } | ||
| .option-area { | ||
| width: 100%; | ||
| height: calc(100vh - 250px); | ||
| overflow: auto; | ||
| } | ||
| .engine-column { | ||
| padding: 0px 5px; | ||
| width: 50%; | ||
| } | ||
| .option { | ||
| padding: 5px 0px; | ||
| border-bottom: 1px solid var(--text-separator-color); | ||
| } | ||
| .controls { | ||
| justify-content: center; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance styles for better responsiveness and maintainability
- Use more flexible units for better responsiveness:
.form-group {
width: 100%;
- max-width: 600px;
+ max-width: min(600px, 90vw);
}
.option-area {
width: 100%;
- height: calc(100vh - 250px);
+ max-height: calc(100vh - 250px);
overflow: auto;
}
.engine-column {
- padding: 0px 5px;
+ padding: 0 0.5rem;
width: 50%;
}
.option {
- padding: 5px 0px;
+ padding: 0.5rem 0;
border-bottom: 1px solid var(--text-separator-color);
}- Use CSS custom properties for better maintainability:
:root {
--form-max-width: 600px;
--option-area-max-height: calc(100vh - 250px);
--engine-column-padding: 0.5rem;
--option-padding: 0.5rem;
}
.form-group {
max-width: min(var(--form-max-width), 90vw);
}
.option-area {
max-height: var(--option-area-max-height);
}
.engine-column {
padding: 0 var(--engine-column-padding);
}
.option {
padding: var(--option-padding) 0;
}- Add media queries for better mobile support:
@media (max-width: 480px) {
.engine-column {
width: 100%;
}
.controls {
flex-direction: column;
align-items: center;
}
.controls button {
margin-bottom: 0.5rem;
}
}These changes will improve the responsiveness and maintainability of your styles.
| <template> | ||
| <div> | ||
| <dialog ref="dialog"> | ||
| <div class="title">{{ t.compareEngineSettings }}</div> | ||
| <div class="form-group"> | ||
| <div class="select-area row"> | ||
| <div class="engine-column"> | ||
| <PlayerSelector | ||
| player-uri="" | ||
| :engines="usiEngines" | ||
| :enable-edit-button="false" | ||
| @select-player="selectLeft" | ||
| /> | ||
| </div> | ||
| <div class="engine-column"> | ||
| <PlayerSelector | ||
| player-uri="" | ||
| :engines="usiEngines" | ||
| :enable-edit-button="false" | ||
| @select-player="selectRight" | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div class="option-area"> | ||
| <div v-if="leftEngineURI === '' || rightEngineURI === ''"> | ||
| {{ t.pleaseSelectEngines }} | ||
| </div> | ||
| <div v-else-if="diff.length === 0">{{ t.noDifference }}</div> | ||
| <div v-for="option of diff" :key="option.name" class="option"> | ||
| <div> | ||
| <span>{{ option.displayName }}</span> | ||
| <span v-if="option.displayName"> (</span> | ||
| <span>{{ option.name }}</span> | ||
| <span v-if="option.displayName">)</span> | ||
| </div> | ||
| <div class="row"> | ||
| <div class="engine-column">{{ option.leftValue }}</div> | ||
| <div class="engine-column">{{ option.rightValue }}</div> | ||
| </div> | ||
| <div v-if="!option.mergeable">{{ t.thisItemCannotBeMerged }}</div> | ||
| <div v-else class="row controls"> | ||
| <button @click="mergeToLeft(option.name)"> | ||
| <Icon :icon="IconType.BACK" /> | ||
| {{ t.mergeToLeft }} | ||
| </button> | ||
| <button @click="mergeToRight(option.name)"> | ||
| {{ t.mergeToRight }} | ||
| <Icon :icon="IconType.NEXT" /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="main-buttons"> | ||
| <button data-hotkey="Enter" autofocus @click="ok()"> | ||
| {{ t.ok }} | ||
| </button> | ||
| <button data-hotkey="Escape" @click="cancel()"> | ||
| {{ t.cancel }} | ||
| </button> | ||
| </div> | ||
| </dialog> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience in the template
While the overall structure is good, consider the following improvements:
- Add ARIA labels to improve accessibility:
-<dialog ref="dialog">
+<dialog ref="dialog" aria-labelledby="dialog-title">
- <div class="title">{{ t.compareEngineSettings }}</div>
+ <div id="dialog-title" class="title">{{ t.compareEngineSettings }}</div>- Use semantic HTML elements for better structure:
-<div class="title">{{ t.compareEngineSettings }}</div>
+<h2 id="dialog-title" class="title">{{ t.compareEngineSettings }}</h2>- Add descriptive text for screen readers to the merge buttons:
-<button @click="mergeToLeft(option.name)">
+<button @click="mergeToLeft(option.name)" :aria-label="`${t.mergeToLeft} ${option.name}`">
<Icon :icon="IconType.BACK" />
{{ t.mergeToLeft }}
</button>These changes will improve the accessibility and semantic structure of your component.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <template> | |
| <div> | |
| <dialog ref="dialog"> | |
| <div class="title">{{ t.compareEngineSettings }}</div> | |
| <div class="form-group"> | |
| <div class="select-area row"> | |
| <div class="engine-column"> | |
| <PlayerSelector | |
| player-uri="" | |
| :engines="usiEngines" | |
| :enable-edit-button="false" | |
| @select-player="selectLeft" | |
| /> | |
| </div> | |
| <div class="engine-column"> | |
| <PlayerSelector | |
| player-uri="" | |
| :engines="usiEngines" | |
| :enable-edit-button="false" | |
| @select-player="selectRight" | |
| /> | |
| </div> | |
| </div> | |
| <div class="option-area"> | |
| <div v-if="leftEngineURI === '' || rightEngineURI === ''"> | |
| {{ t.pleaseSelectEngines }} | |
| </div> | |
| <div v-else-if="diff.length === 0">{{ t.noDifference }}</div> | |
| <div v-for="option of diff" :key="option.name" class="option"> | |
| <div> | |
| <span>{{ option.displayName }}</span> | |
| <span v-if="option.displayName"> (</span> | |
| <span>{{ option.name }}</span> | |
| <span v-if="option.displayName">)</span> | |
| </div> | |
| <div class="row"> | |
| <div class="engine-column">{{ option.leftValue }}</div> | |
| <div class="engine-column">{{ option.rightValue }}</div> | |
| </div> | |
| <div v-if="!option.mergeable">{{ t.thisItemCannotBeMerged }}</div> | |
| <div v-else class="row controls"> | |
| <button @click="mergeToLeft(option.name)"> | |
| <Icon :icon="IconType.BACK" /> | |
| {{ t.mergeToLeft }} | |
| </button> | |
| <button @click="mergeToRight(option.name)"> | |
| {{ t.mergeToRight }} | |
| <Icon :icon="IconType.NEXT" /> | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="main-buttons"> | |
| <button data-hotkey="Enter" autofocus @click="ok()"> | |
| {{ t.ok }} | |
| </button> | |
| <button data-hotkey="Escape" @click="cancel()"> | |
| {{ t.cancel }} | |
| </button> | |
| </div> | |
| </dialog> | |
| </div> | |
| </template> | |
| <template> | |
| <div> | |
| <dialog ref="dialog" aria-labelledby="dialog-title"> | |
| <h2 id="dialog-title" class="title">{{ t.compareEngineSettings }}</h2> | |
| <div class="form-group"> | |
| <div class="select-area row"> | |
| <div class="engine-column"> | |
| <PlayerSelector | |
| player-uri="" | |
| :engines="usiEngines" | |
| :enable-edit-button="false" | |
| @select-player="selectLeft" | |
| /> | |
| </div> | |
| <div class="engine-column"> | |
| <PlayerSelector | |
| player-uri="" | |
| :engines="usiEngines" | |
| :enable-edit-button="false" | |
| @select-player="selectRight" | |
| /> | |
| </div> | |
| </div> | |
| <div class="option-area"> | |
| <div v-if="leftEngineURI === '' || rightEngineURI === ''"> | |
| {{ t.pleaseSelectEngines }} | |
| </div> | |
| <div v-else-if="diff.length === 0">{{ t.noDifference }}</div> | |
| <div v-for="option of diff" :key="option.name" class="option"> | |
| <div> | |
| <span>{{ option.displayName }}</span> | |
| <span v-if="option.displayName"> (</span> | |
| <span>{{ option.name }}</span> | |
| <span v-if="option.displayName">)</span> | |
| </div> | |
| <div class="row"> | |
| <div class="engine-column">{{ option.leftValue }}</div> | |
| <div class="engine-column">{{ option.rightValue }}</div> | |
| </div> | |
| <div v-if="!option.mergeable">{{ t.thisItemCannotBeMerged }}</div> | |
| <div v-else class="row controls"> | |
| <button @click="mergeToLeft(option.name)" :aria-label="`${t.mergeToLeft} ${option.name}`"> | |
| <Icon :icon="IconType.BACK" /> | |
| {{ t.mergeToLeft }} | |
| </button> | |
| <button @click="mergeToRight(option.name)"> | |
| {{ t.mergeToRight }} | |
| <Icon :icon="IconType.NEXT" /> | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="main-buttons"> | |
| <button data-hotkey="Enter" autofocus @click="ok()"> | |
| {{ t.ok }} | |
| </button> | |
| <button data-hotkey="Escape" @click="cancel()"> | |
| {{ t.cancel }} | |
| </button> | |
| </div> | |
| </dialog> | |
| </div> | |
| </template> |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/tests/mock/research.ts (1)
17-18: LGTM. Consider refactoring for DRY principle.The change to use
testUSIEnginefor both the mainusiproperty and thesecondariesarray is consistent and appropriate. This ensures uniformity in the mock data for testing purposes.Consider refactoring to reduce repetition:
const engineConfig = { usi: testUSIEngine }; export const researchSettingsSecondaryEngines: ResearchSettings = { ...engineConfig, secondaries: [engineConfig, engineConfig], enableMaxSeconds: false, maxSeconds: 5, };This approach would make future updates easier and reduce the chance of inconsistencies.
src/renderer/view/dialog/USIEngineMergeDialog.vue (3)
3-4: Enhance dialog accessibilityConsider adding ARIA attributes to improve the dialog's accessibility:
-<dialog ref="dialog"> +<dialog ref="dialog" role="dialog" aria-labelledby="dialog-title"> - <div class="title">{{ t.compareEngineSettings }}</div> + <h2 id="dialog-title" class="title">{{ t.compareEngineSettings }}</h2>This change provides better semantic structure and improves screen reader support.
57-60: Improve merge button accessibilityAdd more descriptive labels for screen readers to the merge buttons:
-<button @click="mergeToLeft(option.name)"> +<button @click="mergeToLeft(option.name)" :aria-label="`${t.mergeToLeft} ${option.name}`"> <Icon :icon="IconType.BACK" /> {{ t.mergeToLeft }} </button> -<button @click="mergeToRight(option.name)"> +<button @click="mergeToRight(option.name)" :aria-label="`${t.mergeToRight} ${option.name}`"> {{ t.mergeToRight }} <Icon :icon="IconType.NEXT" /> </button>This change provides more context for screen reader users about which option is being merged.
Also applies to: 63-66
95-100: Enhance type safety for engines propConsider adding a validator function to ensure the
enginesprop is of the correct type:const props = defineProps({ engines: { type: Object as PropType<ImmutableUSIEngines>, required: true, validator: (value: unknown): value is ImmutableUSIEngines => value instanceof ImmutableUSIEngines, }, });This addition provides stronger type checking at runtime.
src/tests/background/usi/index.spec.ts (1)
142-144: LGTM! Consider extracting the extended engine configuration.The use of
testUSIEnginewith object spread is consistent with previous changes and allows for easy extension. TheenableEarlyPonderproperty is added specifically for this test case.Consider extracting this extended engine configuration to a separate constant or helper function if it's used in multiple test cases. This would improve reusability and make the test setup more explicit. For example:
const createEarlyPonderEngine = (baseEngine) => ({ ...baseEngine, enableEarlyPonder: true, }); // Usage in test const setupPromise = setupPlayer(createEarlyPonderEngine(testUSIEngine), 10);src/tests/common/settings/usi.spec.ts (1)
361-397: LGTM: Comprehensive test case for compareUSIEngineOptionsThe new test case for
compareUSIEngineOptionsis well-structured and covers a wide range of scenarios, including different value types, missing options, and type mismatches. It effectively tests the function's ability to compare and determine mergeability of USI engine options.Consider adding a comment explaining the purpose of each option in the test data to improve readability and maintainability. For example:
const lhs: USIEngine = { ...testUSIEngine, options: { USI_Hash: { name: "USI_Hash", type: "spin", order: 0, default: 32, value: 64 }, // Different value USI_Ponder: { name: "USI_Ponder", type: "check", order: 1, default: "true" }, // Same in both Refresh: { name: "Refresh", type: "button", order: 2 }, // Button type, should be skipped // ... (continue for other options) }, };This would make it easier for other developers to understand the purpose of each test case at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/common/settings/usi.ts (1 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
- src/tests/background/usi/index.spec.ts (4 hunks)
- src/tests/common/settings/usi.spec.ts (2 hunks)
- src/tests/mock/research.ts (1 hunks)
- src/tests/mock/usi.ts (2 hunks)
- src/tests/renderer/players/usi.spec.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (19)
src/tests/mock/research.ts (3)
5-5: LGTM. Consistent use oftestUSIEngine.The change from
usiEnginestotestUSIEnginein bothresearchSettingsandresearchSettingsMax5Secondsis consistent and aligns with the updated import statement.Also applies to: 11-11
1-21: Overall, the changes look good and are consistent.The modifications in this file align well with the purpose of the pull request. The switch from
usiEnginestotestUSIEngineis applied consistently throughout the file, which should improve the specificity of the mocks used in testing. The suggested refactoring for theresearchSettingsSecondaryEnginesconstant could further improve code maintainability.
2-2: LGTM. Verify the new import across the codebase.The change from
usiEnginestotestUSIEngineis consistent with the modifications in the rest of the file. This likely provides a more specific mock for testing purposes.Let's verify that
testUSIEngineis correctly defined and used:✅ Verification successful
Import Verification Successful
The
testUSIEngineis correctly defined insrc/tests/mock/usi.ts, and there are no remaining usages ofusiEnginesin the test files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of testUSIEngine # Test 1: Check if testUSIEngine is exported from mock/usi.ts rg -p 'export.*testUSIEngine' src/tests/mock/usi.ts # Test 2: Check for any remaining usage of usiEngines in test files rg 'usiEngines' src/tests/Length of output: 179
src/tests/mock/usi.ts (3)
Line range hint
3-13: Improved naming for test USI engine constantThe renaming of
usiEnginestotestUSIEngineenhances clarity and accurately represents the constant's purpose in the testing context. The singular form is more appropriate as it represents a single engine. This change aligns well with best practices for naming test fixtures.
Line range hint
14-31: Consistent renaming for test USI engine with ponderThe renaming of
usiEnginesWithPondertotestUSIEngineWithPondermaintains consistency with the previous constant's renaming. This change enhances the overall clarity of the test fixtures and accurately represents the constant's purpose, including its ponder functionality.
Line range hint
1-31: Verify consistency of renamed constants across the codebaseThe renaming of both constants improves clarity and accurately represents their purpose as test fixtures. To ensure the consistency of these changes:
- Verify that all references to the old constant names (
usiEnginesandusiEnginesWithPonder) have been updated in other test files.- Check for any potential impacts on test coverage or test behavior due to these renamings.
Run the following script to check for any remaining references to the old constant names:
If any results are found, please update those references to use the new constant names.
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-198: Overall assessment: Well-structured component with room for improvementThis new
USIEngineMergeDialogcomponent is well-structured and follows Vue 3 best practices. It effectively implements the required functionality for comparing and merging USI engine settings. Here's a summary of the main points and areas for improvement:
- Accessibility: Enhance dialog and button accessibility for better screen reader support.
- Performance: Consider memoizing the
diffcomputed property for better performance with large datasets.- Type safety: Strengthen type checking for the
enginesprop.- Responsive design: Improve the styles to be more adaptable to different screen sizes.
Implementing these suggestions will further improve the component's quality, accessibility, and maintainability.
src/tests/background/usi/index.spec.ts (4)
94-94: LGTM! Consistent use oftestUSIEngine.The
setupPlayerfunction is now correctly usingtestUSIEngine, which is consistent with the earlier import statement change. The rest of the test logic remains intact, preserving the test's integrity.
172-172: LGTM! Consistent use oftestUSIEngine.The
setupPlayerfunction is now correctly usingtestUSIEngine, which is consistent with the earlier changes in the file. The rest of the test logic remains intact, preserving the test's integrity.
Line range hint
1-193: Summary: Consistent refactoring to usetestUSIEngineThe changes in this file are part of a larger refactoring effort to replace
usiEngineswithtestUSIEngine. This modification has been consistently applied throughout the file, affecting the import statement and three test functions: "go", "early-ponder", and "activeSessionCount".Key points:
- The changes maintain the integrity of the tests, as only the engine reference is updated.
- The test logic and structure remain unchanged, ensuring that the tests continue to validate the same behaviors.
- The modifications align with the AI-generated summary and improve naming consistency in the project.
Overall, these changes appear to be a positive step towards better code organization and clarity.
16-16: LGTM! Verify consistent usage oftestUSIEngine.The import statement has been updated from
usiEnginestotestUSIEngine, which aligns with the refactoring mentioned in the summary. This change improves naming consistency.To ensure this change is applied consistently, run the following script:
✅ Verification successful
Consistent renaming of
usiEnginestotestUSIEngineverified.All instances have been updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of testUSIEngine and absence of usiEngines # Test 1: Check for any remaining instances of usiEngines echo "Checking for any remaining instances of usiEngines:" rg 'usiEngines' src/tests/background/usi/index.spec.ts # Test 2: Verify correct usage of testUSIEngine echo "Verifying correct usage of testUSIEngine:" rg 'testUSIEngine' src/tests/background/usi/index.spec.tsLength of output: 508
src/tests/renderer/players/usi.spec.ts (5)
4-4: Improved naming convention for test engines.The renaming of
usiEnginestotestUSIEngineandusiEnginesWithPondertotestUSIEngineWithPonderenhances code clarity by explicitly indicating these are test-specific mock engines. This change improves readability and maintainability without altering the test functionality.
41-41: Consistent use of renamed test engine.The USIPlayer instantiation has been updated to use
testUSIEngineWithPonder, maintaining consistency with the import statement change. This ensures that the test continues to use the correct mock engine with ponder functionality.
104-104: Consistent renaming across test cases.The USIPlayer instantiation in this test case has also been updated to use
testUSIEngineWithPonder, maintaining consistency across all test cases. This change ensures that all tests are using the correctly renamed mock engine with ponder functionality.
131-131: Appropriate use of non-ponder test engine.The USIPlayer instantiation in this test case has been updated to use
testUSIEngine, which is the non-ponder version of the mock engine. This change is consistent with the import statement modification and is appropriate for this specific test case that doesn't require ponder functionality.
Line range hint
4-131: Summary: Consistent renaming of mock engines improves test clarity.The changes in this file are focused on renaming the mock engines used in the USIPlayer tests. The renaming from
usiEnginestotestUSIEngineandusiEnginesWithPondertotestUSIEngineWithPonderhas been consistently applied throughout the file. These changes enhance code readability and maintainability by clearly indicating the test-specific nature of these mock engines.Key points:
- The import statement has been updated to reflect the new names.
- All USIPlayer instantiations now use the renamed mock engines.
- The appropriate engine (with or without ponder) is used in each test case.
- No functional changes were made to the tests themselves, preserving their original intent and coverage.
These refactoring changes improve the overall quality of the test suite without altering its behavior or coverage.
src/tests/common/settings/usi.spec.ts (1)
Line range hint
1-13: LGTM: Import statements updated correctlyThe import statements have been appropriately updated to include
compareUSIEngineOptionsandtestUSIEngine. These changes align with the new test case and the refactoring of mock data.src/common/settings/usi.ts (2)
200-205: Addition ofUSIEngineOptionDifftype is well-defined.The new type
USIEngineOptionDiffis properly defined and follows TypeScript best practices.
207-249: Implementation ofcompareUSIEngineOptionsis clear and effective.The
compareUSIEngineOptionsfunction efficiently compares engine options between twoUSIEngineinstances. The logic correctly handles different cases of option presence and value differences.
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const translate = appSettings.translateEngineOptionName; | ||
| return compareUSIEngineOptions(left, right).map((option) => { | ||
| return { | ||
| ...option, | ||
| displayName: translate ? usiOptionNameMap[option.name] : "", | ||
| }; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize diff computation performance
Consider memoizing the diff computed property to avoid unnecessary recalculations:
import { computed, ref, watch } from 'vue';
const memoizedDiff = computed(() => {
// ... existing diff logic
});
const cachedDiff = ref(memoizedDiff.value);
watch(memoizedDiff, (newValue) => {
cachedDiff.value = newValue;
});Then use cachedDiff in your template instead of diff. This can improve performance, especially when dealing with a large number of engine options.
| function compareOrder(a: USIEngineOption, b: USIEngineOption): number { | ||
| return a.order < b.order ? -1 : 1; | ||
| } |
There was a problem hiding this comment.
Modify compareOrder function to handle equal order values correctly.
The current compareOrder function does not account for cases where a.order === b.order, which may lead to inconsistent sorting results.
Consider updating the function as follows to properly handle equal order values:
-function compareOrder(a: USIEngineOption, b: USIEngineOption): number {
- return a.order < b.order ? -1 : 1;
+function compareOrder(a: USIEngineOption, b: USIEngineOption): number {
+ return a.order - b.order;
}This change ensures that when a.order === b.order, the function returns 0, leading to consistent sorting behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function compareOrder(a: USIEngineOption, b: USIEngineOption): number { | |
| return a.order < b.order ? -1 : 1; | |
| } | |
| function compareOrder(a: USIEngineOption, b: USIEngineOption): number { | |
| return a.order - b.order; | |
| } |
sunfish-shogi
force-pushed
the
add-merge-dialog
branch
from
095ad00
to
abace1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/renderer/view/dialog/USIEngineManagementDialog.vue (1)
Line range hint
204-247: Improve error handling in mergeOk methodThe implementation of
openMerge,mergeOk, andmergeCancelmethods is generally correct and aligns with the PR objectives. However, themergeOkmethod could benefit from additional error handling to prevent potential runtime errors.Consider adding a check to ensure
enginesis valid before callinggetClone():const mergeOk = (engines: ImmutableUSIEngines) => { if (engines && typeof engines.getClone === 'function') { usiEngines.value = engines.getClone(); mergeDialog.value = false; } else { console.error('Invalid engines object received in mergeOk'); mergeDialog.value = false; } };This change will prevent potential runtime errors if
enginesisundefined,null, or doesn't have agetClonemethod.src/common/i18n/locales/zh_tw.ts (1)
501-502: Consider adding a TODO comment for consistencyWhile the addition of Japanese placeholder text for new translation keys is acceptable, consider adding a TODO comment to these lines for consistency with the previous translations (lines 340-344).
- pleaseSelectEngines: "エンジンを選択してください。", - thisItemCannotBeMerged: "この項目はマージできません。", + pleaseSelectEngines: "エンジンを選択してください。", // TODO: translate + thisItemCannotBeMerged: "この項目はマージできません。", // TODO: translate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- src/common/i18n/locales/en.ts (2 hunks)
- src/common/i18n/locales/ja.ts (2 hunks)
- src/common/i18n/locales/vi.ts (2 hunks)
- src/common/i18n/locales/zh_tw.ts (2 hunks)
- src/common/i18n/text_template.ts (2 hunks)
- src/common/settings/usi.ts (1 hunks)
- src/renderer/view/dialog/PlayerSelector.vue (2 hunks)
- src/renderer/view/dialog/USIEngineManagementDialog.vue (6 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
- src/tests/background/usi/index.spec.ts (4 hunks)
- src/tests/common/settings/usi.spec.ts (2 hunks)
- src/tests/mock/research.ts (1 hunks)
- src/tests/mock/usi.ts (2 hunks)
- src/tests/renderer/players/usi.spec.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/common/i18n/locales/en.ts
- src/common/i18n/locales/ja.ts
- src/common/i18n/locales/vi.ts
- src/common/settings/usi.ts
- src/renderer/view/dialog/PlayerSelector.vue
- src/tests/background/usi/index.spec.ts
- src/tests/common/settings/usi.spec.ts
- src/tests/mock/research.ts
- src/tests/mock/usi.ts
- src/tests/renderer/players/usi.spec.ts
🧰 Additional context used
📓 Learnings (1)
src/common/i18n/locales/zh_tw.ts (2)
Learnt from: sunfish-shogi PR: sunfish-shogi/electron-shogi#980 File: src/common/i18n/locales/zh_tw.ts:340-344 Timestamp: 2024-10-20T05:15:47.654Z Learning: When TODO comments are added for translations in localization files, and the translations are temporarily in another language, this is intentional and should not be flagged as an issue.Learnt from: sunfish-shogi PR: sunfish-shogi/electron-shogi#980 File: src/common/i18n/locales/vi.ts:343-347 Timestamp: 2024-10-20T05:16:48.137Z Learning: In localization files (e.g., `src/common/i18n/locales/vi.ts`), when new translation keys are added with placeholder text (e.g., Japanese) and accompanied by TODO comments for translation, it's acceptable, and we should not flag this as an issue.
🔇 Additional comments (15)
src/renderer/view/dialog/USIEngineMergeDialog.vue (1)
1-198: Overall assessment: Well-structured component with room for improvementsThe
USIEngineMergeDialogcomponent is well-structured and implements the required functionality for comparing and merging USI engine settings. However, there are several areas where the component can be improved:
Accessibility: Enhance the template with ARIA attributes and semantic HTML elements to improve screen reader support and overall accessibility.
Performance: Optimize the
diffcomputed property by memoizing its result to avoid unnecessary recalculations, especially when dealing with a large number of engine options.Type safety: Improve type annotations, especially for props and ref declarations, to enhance type checking and reduce potential runtime errors.
Responsiveness: Refactor the styles to use more flexible units, CSS variables, and media queries to ensure the component adapts well to different screen sizes.
Code organization: Consider extracting some logic, such as the merge functions, into separate, reusable functions to improve readability and maintainability.
By implementing these suggestions, you can significantly improve the quality, performance, and user experience of the
USIEngineMergeDialogcomponent.src/renderer/view/dialog/USIEngineManagementDialog.vue (5)
60-63: LGTM: New "Compare and Merge" button addedThe addition of the "Compare and Merge" button is well-implemented and consistent with the existing UI. It correctly triggers the
openMerge()method, which aligns with the PR objective of adding USIEngineMergeDialog functionality.
80-85: LGTM: USIEngineMergeDialog component integrationThe
USIEngineMergeDialogcomponent is well-integrated into the template. It's correctly conditional rendered based on themergeDialogref, and properly receives theenginesprop while emittingokandcancelevents. This implementation aligns with Vue best practices and the PR objectives.
92-98: LGTM: Updated import statementThe import statement has been correctly updated to include
ImmutableUSIEngines. This addition is necessary for the new merge functionality and is properly placed with other related imports from the same module.
108-108: LGTM: Import USIEngineMergeDialog componentThe
USIEngineMergeDialogcomponent is correctly imported. This import is necessary for using the new component in the template and is properly placed with other component imports.
114-114: LGTM: Add mergeDialog refThe
mergeDialogref is correctly implemented usingref(false). This reactive reference is essential for controlling the visibility of theUSIEngineMergeDialogcomponent, and its initial value offalseensures the dialog is closed by default.src/common/i18n/text_template.ts (7)
338-338: LGTM: New propertycompareAndMergeaddedThe new property
compareAndMergehas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text related to comparing and merging engine settings.
339-339: LGTM: New propertycompareEngineSettingsaddedThe new property
compareEngineSettingshas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text related to comparing engine settings.
340-340: LGTM: New propertynoDifferenceaddedThe new property
noDifferencehas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text indicating no differences found when comparing engine settings.
341-341: LGTM: New propertymergeToLeftaddedThe new property
mergeToLefthas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text related to merging settings to the left side in a comparison view.
342-342: LGTM: New propertymergeToRightaddedThe new property
mergeToRighthas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text related to merging settings to the right side in a comparison view.
499-499: LGTM: New propertypleaseSelectEnginesaddedThe new property
pleaseSelectEngineshas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text prompting the user to select engines for comparison or merging.
500-500: LGTM: New propertythisItemCannotBeMergedadded and grammatical error fixedThe new property
thisItemCannotBeMergedhas been correctly added to theTextstype. It follows the proper naming convention and is likely used for UI text indicating that a particular item cannot be merged. The grammatical error pointed out in the previous review has been addressed by changingthisItemCannotBeMergetothisItemCannotBeMerged.src/common/i18n/locales/zh_tw.ts (2)
340-344: LGTM: Placeholder translations added with TODO commentThe addition of new translation keys with Japanese placeholder text and a TODO comment is appropriate. This approach allows for native speakers to provide accurate translations in the future.
Line range hint
1-502: LGTM: Overall localization file structure and contentThe Traditional Chinese localization file is well-structured and contains appropriate translations for most entries. The use of TODO comments and placeholder text for pending translations is consistent with the project's localization approach.
| <script setup lang="ts"> | ||
| import { computed, onBeforeUnmount, onMounted, PropType, reactive, ref } from "vue"; | ||
| import PlayerSelector from "./PlayerSelector.vue"; | ||
| import { compareUSIEngineOptions, ImmutableUSIEngines } from "@/common/settings/usi"; | ||
| import { showModalDialog } from "@/renderer/helpers/dialog"; | ||
| import { installHotKeyForDialog, uninstallHotKeyForDialog } from "@/renderer/devices/hotkey"; | ||
| import { t, usiOptionNameMap } from "@/common/i18n"; | ||
| import Icon from "@/renderer/view/primitive/Icon.vue"; | ||
| import { IconType } from "@/renderer/assets/icons"; | ||
| import { useAppSettings } from "@/renderer/store/settings"; | ||
|
|
||
| const props = defineProps({ | ||
| engines: { | ||
| type: Object as PropType<ImmutableUSIEngines>, | ||
| required: true, | ||
| }, | ||
| }); | ||
|
|
||
| const emit = defineEmits<{ | ||
| ok: [engines: ImmutableUSIEngines]; | ||
| cancel: []; | ||
| }>(); | ||
|
|
||
| const appSettings = useAppSettings(); | ||
| const dialog = ref(); | ||
| const usiEngines = reactive(props.engines.getClone()); | ||
| const leftEngineURI = ref(""); | ||
| const rightEngineURI = ref(""); | ||
|
|
||
| onMounted(async () => { | ||
| showModalDialog(dialog.value, cancel); | ||
| installHotKeyForDialog(dialog.value); | ||
| }); | ||
|
|
||
| onBeforeUnmount(() => { | ||
| uninstallHotKeyForDialog(dialog.value); | ||
| }); | ||
|
|
||
| const selectLeft = (uri: string) => { | ||
| leftEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const selectRight = (uri: string) => { | ||
| rightEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const mergeToLeft = (name: string) => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return; | ||
| } | ||
| left.options[name] = right.options[name]; | ||
| }; | ||
|
|
||
| const mergeToRight = (name: string) => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return; | ||
| } | ||
| right.options[name] = left.options[name]; | ||
| }; | ||
|
|
||
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const translate = appSettings.translateEngineOptionName; | ||
| return compareUSIEngineOptions(left, right).map((option) => { | ||
| return { | ||
| ...option, | ||
| displayName: translate ? usiOptionNameMap[option.name] : "", | ||
| }; | ||
| }); | ||
| }); | ||
|
|
||
| const ok = () => { | ||
| emit("ok", usiEngines); | ||
| }; | ||
| const cancel = () => { | ||
| emit("cancel"); | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance and enhance type safety in the script section
The script section is well-structured, but consider the following improvements for better performance and type safety:
- Memoize the
diffcomputed property to avoid unnecessary recalculations:
import { computed, ref, watch } from 'vue';
const memoizedDiff = computed(() => {
// ... existing diff logic
});
const cachedDiff = ref(memoizedDiff.value);
watch(memoizedDiff, (newValue) => {
cachedDiff.value = newValue;
});Then use cachedDiff in your template instead of diff.
- Use more specific types for the
enginesprop:
import { PropType } from 'vue';
const props = defineProps({
engines: {
type: Object as PropType<ImmutableUSIEngines>,
required: true,
validator: (value: unknown): value is ImmutableUSIEngines =>
value instanceof ImmutableUSIEngines,
},
});- Consider using
shallowReffor large objects that don't need deep reactivity:
import { shallowRef } from 'vue';
const usiEngines = shallowRef(props.engines.getClone());- Add type annotations to ref declarations:
const leftEngineURI = ref<string>("");
const rightEngineURI = ref<string>("");- Consider extracting the merge logic into a separate function to improve readability:
const mergeOption = (fromURI: string, toURI: string, optionName: string) => {
const fromEngine = usiEngines.getEngine(fromURI);
const toEngine = usiEngines.getEngine(toURI);
if (!fromEngine || !toEngine) return;
toEngine.options[optionName] = fromEngine.options[optionName];
};
const mergeToLeft = (name: string) => mergeOption(rightEngineURI.value, leftEngineURI.value, name);
const mergeToRight = (name: string) => mergeOption(leftEngineURI.value, rightEngineURI.value, name);These changes will improve the performance, type safety, and maintainability of your component.
sunfish-shogi
force-pushed
the
add-merge-dialog
branch
from
abace1a
to
a1a5d01
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- src/common/i18n/locales/en.ts (2 hunks)
- src/common/i18n/locales/ja.ts (2 hunks)
- src/common/i18n/locales/vi.ts (2 hunks)
- src/common/i18n/locales/zh_tw.ts (2 hunks)
- src/common/i18n/text_template.ts (2 hunks)
- src/common/settings/usi.ts (1 hunks)
- src/renderer/view/dialog/PlayerSelector.vue (2 hunks)
- src/renderer/view/dialog/USIEngineManagementDialog.vue (6 hunks)
- src/renderer/view/dialog/USIEngineMergeDialog.vue (1 hunks)
- src/tests/background/usi/index.spec.ts (4 hunks)
- src/tests/common/settings/usi.spec.ts (2 hunks)
- src/tests/mock/research.ts (1 hunks)
- src/tests/mock/usi.ts (2 hunks)
- src/tests/renderer/players/usi.spec.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/common/i18n/locales/en.ts
- src/common/i18n/locales/ja.ts
- src/common/i18n/locales/vi.ts
- src/common/i18n/locales/zh_tw.ts
- src/common/i18n/text_template.ts
- src/common/settings/usi.ts
- src/renderer/view/dialog/PlayerSelector.vue
- src/renderer/view/dialog/USIEngineManagementDialog.vue
- src/tests/background/usi/index.spec.ts
- src/tests/common/settings/usi.spec.ts
- src/tests/mock/research.ts
- src/tests/mock/usi.ts
- src/tests/renderer/players/usi.spec.ts
🧰 Additional context used
🔇 Additional comments (3)
src/renderer/view/dialog/USIEngineMergeDialog.vue (3)
1-82: LGTM: Well-structured template with clear logicThe overall structure and logic of the template are well-organized and follow Vue.js best practices. The use of conditional rendering for different states (engine selection, no differences, and option comparisons) is appropriate and enhances user experience. The component effectively utilizes sub-components like PlayerSelector and provides a clear interface for comparing and merging engine settings.
84-171: LGTM: Well-structured script with clear component logicThe script section is well-organized and effectively uses Vue 3 Composition API. The component logic, including engine selection, merging, and diff computation, is clearly implemented. The use of lifecycle hooks for dialog management and hotkey installation is appropriate. The overall structure promotes maintainability and readability of the code.
173-200: LGTM: Appropriate use of scoped stylesThe use of scoped styles is a good practice to prevent style leaks and maintain component encapsulation. This approach helps in maintaining a clean and predictable styling structure for the component.
| <template> | ||
| <div> | ||
| <dialog ref="dialog"> | ||
| <div class="title">{{ t.compareEngineSettings }}</div> | ||
| <div class="form-group"> | ||
| <div class="select-area row"> | ||
| <div class="engine-column"> | ||
| <PlayerSelector | ||
| player-uri="" | ||
| :engines="usiEngines" | ||
| :enable-edit-button="false" | ||
| @select-player="selectLeft" | ||
| /> | ||
| </div> | ||
| <div class="engine-column"> | ||
| <PlayerSelector | ||
| player-uri="" | ||
| :engines="usiEngines" | ||
| :enable-edit-button="false" | ||
| @select-player="selectRight" | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div class="option-area"> | ||
| <div v-if="leftEngineURI === '' || rightEngineURI === ''"> | ||
| {{ t.pleaseSelectEngines }} | ||
| </div> | ||
| <div v-else-if="diff.length === 0">{{ t.noDifference }}</div> | ||
| <div v-for="option of diff" :key="option.name" class="option"> | ||
| <div> | ||
| <span>{{ option.displayName }}</span> | ||
| <span v-if="option.displayName"> (</span> | ||
| <span>{{ option.name }}</span> | ||
| <span v-if="option.displayName">)</span> | ||
| </div> | ||
| <div class="row"> | ||
| <div class="engine-column"> | ||
| <input | ||
| v-if="option.leftValue" | ||
| class="option-value" | ||
| readonly | ||
| :value="option.leftValue" | ||
| /> | ||
| </div> | ||
| <div class="engine-column"> | ||
| <input | ||
| v-if="option.rightValue" | ||
| class="option-value" | ||
| readonly | ||
| :value="option.rightValue" | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div v-if="!option.mergeable">{{ t.thisItemCannotBeMerged }}</div> | ||
| <div v-else class="row"> | ||
| <div class="engine-column"> | ||
| <button @click="mergeToLeft(option.name)"> | ||
| <Icon :icon="IconType.BACK" /> | ||
| {{ t.mergeToLeft }} | ||
| </button> | ||
| </div> | ||
| <div class="engine-column"> | ||
| <button @click="mergeToRight(option.name)"> | ||
| {{ t.mergeToRight }} | ||
| <Icon :icon="IconType.NEXT" /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="main-buttons"> | ||
| <button data-hotkey="Enter" autofocus @click="ok()"> | ||
| {{ t.ok }} | ||
| </button> | ||
| <button data-hotkey="Escape" @click="cancel()"> | ||
| {{ t.cancel }} | ||
| </button> | ||
| </div> | ||
| </dialog> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for better user experience
The template structure is well-organized, but consider the following accessibility improvements:
- Add ARIA attributes to the dialog for better screen reader support:
-<dialog ref="dialog">
+<dialog ref="dialog" aria-labelledby="dialog-title" aria-describedby="dialog-description">
- <div class="title">{{ t.compareEngineSettings }}</div>
+ <h2 id="dialog-title" class="title">{{ t.compareEngineSettings }}</h2>
+ <p id="dialog-description" class="visually-hidden">{{ t.compareEngineSettingsDescription }}</p>- Add descriptive text for screen readers to the merge buttons:
-<button @click="mergeToLeft(option.name)">
+<button @click="mergeToLeft(option.name)" :aria-label="`${t.mergeToLeft} ${option.displayName || option.name}`">
<Icon :icon="IconType.BACK" />
{{ t.mergeToLeft }}
</button>- Consider adding
aria-liveregions for dynamic content updates:
<div aria-live="polite" class="visually-hidden">
{{ leftEngineURI && rightEngineURI ? t.enginesSelected : t.pleaseSelectEngines }}
</div>These changes will improve the accessibility of your component, making it more inclusive for users with assistive technologies.
| <script setup lang="ts"> | ||
| import { computed, onBeforeUnmount, onMounted, PropType, reactive, ref } from "vue"; | ||
| import PlayerSelector from "./PlayerSelector.vue"; | ||
| import { | ||
| compareUSIEngineOptions, | ||
| getUSIEngineOptionCurrentValue, | ||
| ImmutableUSIEngines, | ||
| } from "@/common/settings/usi"; | ||
| import { showModalDialog } from "@/renderer/helpers/dialog"; | ||
| import { installHotKeyForDialog, uninstallHotKeyForDialog } from "@/renderer/devices/hotkey"; | ||
| import { t, usiOptionNameMap } from "@/common/i18n"; | ||
| import Icon from "@/renderer/view/primitive/Icon.vue"; | ||
| import { IconType } from "@/renderer/assets/icons"; | ||
| import { useAppSettings } from "@/renderer/store/settings"; | ||
|
|
||
| const props = defineProps({ | ||
| engines: { | ||
| type: Object as PropType<ImmutableUSIEngines>, | ||
| required: true, | ||
| }, | ||
| }); | ||
|
|
||
| const emit = defineEmits<{ | ||
| ok: [engines: ImmutableUSIEngines]; | ||
| cancel: []; | ||
| }>(); | ||
|
|
||
| const appSettings = useAppSettings(); | ||
| const dialog = ref(); | ||
| const usiEngines = reactive(props.engines.getClone()); | ||
| const leftEngineURI = ref(""); | ||
| const rightEngineURI = ref(""); | ||
|
|
||
| onMounted(async () => { | ||
| showModalDialog(dialog.value, cancel); | ||
| installHotKeyForDialog(dialog.value); | ||
| }); | ||
|
|
||
| onBeforeUnmount(() => { | ||
| uninstallHotKeyForDialog(dialog.value); | ||
| }); | ||
|
|
||
| const selectLeft = (uri: string) => { | ||
| leftEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const selectRight = (uri: string) => { | ||
| rightEngineURI.value = uri; | ||
| }; | ||
|
|
||
| const merge = (name: string, srcURI: string, dstURI: string) => { | ||
| const src = usiEngines.getEngine(srcURI); | ||
| const dst = usiEngines.getEngine(dstURI); | ||
| if (src && dst && src?.options[name].type !== "button" && dst?.options[name].type !== "button") { | ||
| dst.options[name].value = getUSIEngineOptionCurrentValue(src.options[name]); | ||
| } | ||
| }; | ||
|
|
||
| const mergeToLeft = (name: string) => { | ||
| merge(name, rightEngineURI.value, leftEngineURI.value); | ||
| }; | ||
|
|
||
| const mergeToRight = (name: string) => { | ||
| merge(name, leftEngineURI.value, rightEngineURI.value); | ||
| }; | ||
|
|
||
| const diff = computed(() => { | ||
| const left = usiEngines.getEngine(leftEngineURI.value); | ||
| const right = usiEngines.getEngine(rightEngineURI.value); | ||
| if (!left || !right) { | ||
| return []; | ||
| } | ||
| const translate = appSettings.translateEngineOptionName; | ||
| return compareUSIEngineOptions(left, right).map((option) => { | ||
| return { | ||
| ...option, | ||
| displayName: translate ? usiOptionNameMap[option.name] : "", | ||
| }; | ||
| }); | ||
| }); | ||
|
|
||
| const ok = () => { | ||
| emit("ok", usiEngines); | ||
| }; | ||
| const cancel = () => { | ||
| emit("cancel"); | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance of the diff computed property
Consider memoizing the diff computed property to avoid unnecessary recalculations:
import { computed, ref, watch } from 'vue';
const memoizedDiff = computed(() => {
// ... existing diff logic
});
const cachedDiff = ref(memoizedDiff.value);
watch(memoizedDiff, (newValue) => {
cachedDiff.value = newValue;
});Then use cachedDiff in your template instead of diff. This can improve performance, especially when dealing with a large number of engine options.
| const props = defineProps({ | ||
| engines: { | ||
| type: Object as PropType<ImmutableUSIEngines>, | ||
| required: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety for the engines prop
Consider adding a validator function to ensure the engines prop is of the correct type:
import { PropType } from 'vue';
const props = defineProps({
engines: {
type: Object as PropType<ImmutableUSIEngines>,
required: true,
validator: (value: unknown): value is ImmutableUSIEngines =>
value instanceof ImmutableUSIEngines,
},
});This change will improve type safety and help catch potential errors earlier in the development process.
| <style scoped> | ||
| .form-group { | ||
| width: 100%; | ||
| max-width: 600px; | ||
| } | ||
| .select-area { | ||
| width: 100%; | ||
| } | ||
| .option-area { | ||
| width: 100%; | ||
| height: calc(100vh - 250px); | ||
| overflow: auto; | ||
| } | ||
| .engine-column { | ||
| margin: 0 5px; | ||
| width: calc(50% - 10px); | ||
| } | ||
| .option { | ||
| padding: 5px 0; | ||
| border-bottom: 1px solid var(--text-separator-color); | ||
| } | ||
| .option > *:not(:last-child) { | ||
| margin-bottom: 5px; | ||
| } | ||
| .option-value { | ||
| width: 100%; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance styles for better responsiveness and maintainability
Consider the following improvements to make your styles more responsive and maintainable:
- Use CSS custom properties for better maintainability:
:root {
--form-max-width: 600px;
--option-area-max-height: calc(100vh - 250px);
--engine-column-margin: 5px;
--option-padding: 5px;
}- Use more flexible units for better responsiveness:
.form-group {
width: 100%;
- max-width: 600px;
+ max-width: var(--form-max-width);
}
.option-area {
width: 100%;
- height: calc(100vh - 250px);
+ max-height: var(--option-area-max-height);
overflow: auto;
}
.engine-column {
- margin: 0 5px;
- width: calc(50% - 10px);
+ margin: 0 var(--engine-column-margin);
+ width: calc(50% - 2 * var(--engine-column-margin));
}
.option {
- padding: 5px 0;
+ padding: var(--option-padding) 0;
border-bottom: 1px solid var(--text-separator-color);
}- Add media queries for better mobile support:
@media (max-width: 600px) {
.engine-column {
width: 100%;
margin: var(--engine-column-margin) 0;
}
.option > .row {
flex-direction: column;
}
}These changes will improve the responsiveness and maintainability of your styles, making the component more adaptable to different screen sizes.
説明 / Description
#792
チェックリスト / Checklist
npm testpassednpm run lintwas applied without warnings/docs/webappnot included (except release branch)console.lognot included (except script file)Summary by CodeRabbit
Release Notes
New Features
USIEngineMergeDialogcomponent allows users to compare and merge settings between two engines.Enhancements
Bug Fixes