-
Notifications
You must be signed in to change notification settings - Fork 314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: MenuBarの定義を分散 #2596
refactor: MenuBarの定義を分散 #2596
Conversation
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューしてみました!!!
これ難しいですね!!!!!!!
最終的にseparatorごとに分けてMenuItem[]
を管理することになると思うので、それを見越してデータ構造を提案してみました!!
eslint.config.mjs
Outdated
group: ["electron"], | ||
regex: "^electron(\\/|$)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(判断メモコメントです)
該当箇所でno-restricted-imports
をignoreする方が目的に合ってる・・・・・かも・・・?
と思ったけど、メインプロセス以外から使われるのはelectron
だけだし、これで良さそう!
* refactor: rendererを分離 * fix: preloadエントリのパスを修正
[update snapshots]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ほぼLGTMです!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あと少し感!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reorganizes the MenuBar definitions by distributing them across multiple modules and updating related type definitions and tests. Key changes include:
- Introducing new composable hooks (e.g., useElectronMenuBarData) and updating MenuBar concatenation logic.
- Adjusting type definitions and renaming properties (e.g., replacing “disablreloadingLocked” with “disableWhileReloadingLock”).
- Updating test cases and ESLint/Vite configurations to reflect these refactorings.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/backend/electron/renderer/menuBarData.ts | Introduces electron-specific menu bar data via a new composable. |
tests/unit/lib/arrayHelper.ts | Adds tests for array helper functions with a minor mismatch in test description naming. |
src/components/Menu/MenuBar/menuBarData.ts | Consolidates MenuBar data concatenation with a workaround for type mismatches. |
tests/unit/domain/menuBarData.spec.ts | Provides tests for the new concatMenuBarData behavior. |
src/backend/electron/ipc.ts | Streamlines IPC types by removing obsolete definitions. |
src/helpers/arrayHelper.ts | Exports helper functions with no functional changes. |
src/components/Menu/MenuBar/useCommonMenuBarData.ts | Defines common menu bar data and registers hotkeys; no functional changes aside from refactor. |
src/helpers/typedEntries.ts | Adds a helper function (mapObjectValues) with inline documentation. |
src/components/Talk/menuBarData.ts | Adopts the new MenuBar content type and refactors accordingly. |
src/components/Menu/type.ts | Renames property “disablreloadingLocked” to “disableWhileReloadingLock”. |
eslint.config.js | Updates the electron import rule to a regex-based approach. |
vite.config.ts | Updates the preload input file path to use the renderer preload file. |
src/components/Sing/menuBarData.ts | Refactors menu data for the Sing module to align with the new MenuBar content structures. |
src/components/App.vue | Integrates new menu bar data sources and updates component prop names accordingly. |
src/components/Menu/MenuButton.vue | Corrects property name usage for reloading lock checks. |
Comments suppressed due to low confidence (3)
tests/unit/lib/arrayHelper.ts:21
- The test suite description 'removeNonValue' does not match the function name 'removeNullableAndBoolean'. Consider updating the description for clarity.
describe("removeNonValue", () => {
src/components/Menu/MenuBar/menuBarData.ts:62
- Avoid using '@ts-expect-error' by addressing the underlying type mismatch; consider refining the type definitions for 'itemsArray' and 'itemValue' to remove the need for suppression.
// @ts-expect-error 型が合わないので無視する。
vite.config.ts:129
- Ensure that changing the preload input file path to 'renderer/preload.ts' aligns with Electron's preload requirements, as the removed comment suggests potential implications.
input: "./src/backend/electron/renderer/preload.ts",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たぶんLGTMなのですが、あとでもう1回だけファイルチェックさせていただこうかなと・・・!!!
Co-authored-by: Hiroshiba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!!!!!!!!
お疲れ様でした!!かなり良い感じにファクタリングで来たと思います!!!!!
良いリファクタリング筋トレ(?)になりました・・・!!
TODOコメントいくつかあるのでissue化しておくとメンテナっぽいかもです!
<template>
<ErrorBoundary>
<TooltipProvider disableHoverableContent :delayDuration="500">
<!-- TODO: subMenuDataのpropsを分けず、1つにまとめて渡す -->
<MenuBar
v-if="openedEditor != undefined"
:fileSubMenuData="subMenuData.file"
:editSubMenuData="subMenuData.edit"
:viewSubMenuData="subMenuData.view"
:engineSubMenuData="subMenuData.engine"
:settingSubMenuData="subMenuData.setting"
const store = useStore();
// TODO: useMenuBarData系の関数をcomposableじゃなくする
const commonMenuBarData = useCommonMenuBarData(store);
const talkMenuBarData = useTalkMenuBarData(store);
const singMenuBarData = useSingMenuBarData(store);
const electronMenuBarData = useElectronMenuBarData(store);
↓のissueにぶら下げておくとコミットしやすいかも?
@sevenc-nanashi これ早めにマージ目指せると良いかもです・・・! ( #2596 (review) にコメント書いています) |
内容
MenuBarの定義を分散させます。
Discordはこのへん:https://canary.discord.com/channels/879570910208733277/893889888208977960/1342134282281287750
関連 Issue
スクリーンショット・動画など
(なし)
その他
(なし)