Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom

スマホブラウザ版の UI を改良 #982

Merged
merged 1 commit into from
Oct 25, 2024
Merged

スマホブラウザ版の UI を改良 #982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
<head>
<meta charset="utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
<meta name="viewport" content="width=device-width,initial-scale=1.0" />
<meta
name="viewport"
content="width=device-width,initial-scale=1.0,minimum-scale=1.0,maximum-scale=1.0,user-scalable=no"
/>
<link rel="icon" href="/favicon.png" />
<title>ShogiHome</title>
</head>
Expand Down
5 changes: 4 additions & 1 deletion layout-manager.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
<head>
<meta charset="utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
<meta name="viewport" content="width=device-width,initial-scale=1.0" />
<meta
name="viewport"
content="width=device-width,initial-scale=1.0,maximum-scale=1.0,maximum-scale=1.0,user-scalable=no"
/>
<link rel="icon" href="/favicon.png" />
<title>ShogiHome Layout Manager</title>
</head>
Expand Down
5 changes: 4 additions & 1 deletion prompt.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
<head>
<meta charset="utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
<meta name="viewport" content="width=device-width,initial-scale=1.0" />
<meta
name="viewport"
content="width=device-width,initial-scale=1.0,maximum-scale=1.0,maximum-scale=1.0,user-scalable=no"
/>
<link rel="icon" href="/favicon.png" />
<title>ShogiHome Prompt</title>
</head>
Expand Down
5 changes: 4 additions & 1 deletion src/renderer/App.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@
:pv="store.pvPreview.pv"
@close="store.closePVPreviewDialog()"
/>
<button v-if="!isNative()" class="copyright" @click="openCopyright">&copy;</button>
<!-- PCブラウザの場合のみライセンスへの遷移が無いので、画面の隅にボタンを表示する。 -->
<button v-if="!isNative() && !isMobileWebApp()" class="copyright" @click="openCopyright">
&copy;
</button>
</div>
</template>

Expand Down
6 changes: 6 additions & 0 deletions src/renderer/store/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,12 @@
saveRecordForWebApp(this.record);
clearURLParams();
})
.on("updateComment", () => {
saveRecordForWebApp(this.record);

Check warning on line 160 in src/renderer/store/index.ts

View check run for this annotation

Codecov / codecov/patch

src/renderer/store/index.ts#L160 

Added line #L160 was not covered by tests
})
.on("updateBookmark", () => {
saveRecordForWebApp(this.record);

Check warning on line 163 in src/renderer/store/index.ts

View check run for this annotation

Codecov / codecov/patch

src/renderer/store/index.ts#L163 

Added line #L163 was not covered by tests
})
Comment on lines +159 to +164
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling and debouncing for save operations.

The new event handlers for comments and bookmarks immediately trigger save operations without error handling. This could lead to silent failures and performance issues with rapid updates.

Consider these improvements:

  1. Add error handling:
 .on("updateComment", () => {
-  saveRecordForWebApp(this.record);
+  saveRecordForWebApp(this.record).catch((error) => {
+    useErrorStore().add("Failed to save comment update: " + error);
+  });
 })
  1. Implement debouncing for better performance:
+private debouncedSave = debounce((record: ImmutableRecord) => {
+  saveRecordForWebApp(record).catch((error) => {
+    useErrorStore().add("Failed to save record: " + error);
+  });
+}, 1000);  // Adjust timeout as needed

 .on("updateComment", () => {
-  saveRecordForWebApp(this.record);
+  this.debouncedSave(this.record);
 })

Don't forget to import the debounce utility:

import { debounce } from 'lodash-es';  // or your preferred debouncing utility

.on("updateCustomData", () => {
this.onUpdateCustomDataHandlers.forEach((handler) => handler());
saveRecordForWebApp(this.record);
Expand Down
37 changes: 32 additions & 5 deletions src/renderer/store/record.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@

export type ChangePositionHandler = () => void;
export type UpdateTreeHandler = () => void;
export type UpdateCommentHandler = () => void;
export type UpdateBookmarkHandler = () => void;
export type UpdateCustomDataHandler = () => void;
export type BackupHandler = () => BackupOptions | null | void;

Expand All @@ -307,6 +309,8 @@
private _sourceURL?: string;
private changePositionHandler: ChangePositionHandler | null = null;
private updateTreeHandler: UpdateTreeHandler | null = null;
private updateCommentHandler: UpdateCommentHandler | null = null;
private updateBookmarkHandler: UpdateBookmarkHandler | null = null;
Comment on lines +312 to +313
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using undefined instead of null for handler initialization.

The new handler properties follow the existing pattern, but there's an opportunity to improve type safety.

- private updateCommentHandler: UpdateCommentHandler | null = null;
- private updateBookmarkHandler: UpdateBookmarkHandler | null = null;
+ private updateCommentHandler: UpdateCommentHandler | undefined = undefined;
+ private updateBookmarkHandler: UpdateBookmarkHandler | undefined = undefined;

This aligns with TypeScript best practices and the static analysis suggestion about avoiding confusing void types in unions.

📝 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.

Suggested change
private updateCommentHandler: UpdateCommentHandler | null = null;
private updateBookmarkHandler: UpdateBookmarkHandler | null = null;
private updateCommentHandler: UpdateCommentHandler | undefined = undefined;
private updateBookmarkHandler: UpdateBookmarkHandler | undefined = undefined;
🧰 Tools
🪛 Biome

[error] 313-313: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

private updateCustomDataHandler: UpdateCustomDataHandler | null = null;
private backupHandler: BackupHandler | null = null;

Expand Down Expand Up @@ -627,11 +631,13 @@
updateComment(comment: string): void {
this._record.current.comment = comment;
this._unsaved = true;
this.onUpdateComment();
}

updateBookmark(bookmark: string): void {
this._record.current.bookmark = bookmark;
this._unsaved = true;
this.onUpdateBookmark();

Check warning on line 640 in src/renderer/store/record.ts

View check run for this annotation

Codecov / codecov/patch

src/renderer/store/record.ts#L640 

Added line #L640 was not covered by tests
}

appendComment(add: string, behavior: CommentBehavior): void {
Expand All @@ -654,6 +660,7 @@
break;
}
this._unsaved = true;
this.onUpdateComment();
}

appendSearchComment(
Expand Down Expand Up @@ -788,20 +795,28 @@
this._unsaved = true;
}

on(event: "changePosition", handler: ChangePositionHandler): this;
on(event: "changePosition", handler: () => void): this;
on(event: "updateTree", handler: () => void): this;
on(event: "updateCustomData", handler: UpdateCustomDataHandler): this;
on(event: "updateComment", handler: () => void): this;
on(event: "updateCustomData", handler: () => void): this;
on(event: "updateBookmark", handler: () => void): this;
on(event: "backup", handler: BackupHandler): this;
on(event: string, handler: unknown): this {
switch (event) {
case "changePosition":
this.changePositionHandler = handler as ChangePositionHandler;
this.changePositionHandler = handler as () => void;
break;
case "updateTree":
this.updateTreeHandler = handler as UpdateTreeHandler;
this.updateTreeHandler = handler as () => void;
break;
case "updateComment":
this.updateCommentHandler = handler as () => void;
break;
case "updateBookmark":
this.updateBookmarkHandler = handler as () => void;
break;
case "updateCustomData":
this.updateCustomDataHandler = handler as UpdateCustomDataHandler;
this.updateCustomDataHandler = handler as () => void;
break;
case "backup":
this.backupHandler = handler as BackupHandler;
Expand All @@ -822,6 +837,18 @@
}
}

private onUpdateComment() {
if (this.updateCommentHandler) {
this.updateCommentHandler();
}

Check warning on line 843 in src/renderer/store/record.ts

View check run for this annotation

Codecov / codecov/patch

src/renderer/store/record.ts#L842-L843 

Added lines #L842 - L843 were not covered by tests
}

private onUpdateBookmark() {
if (this.updateBookmarkHandler) {
this.updateBookmarkHandler();
}
}

Check warning on line 850 in src/renderer/store/record.ts

View check run for this annotation

Codecov / codecov/patch

src/renderer/store/record.ts#L847-L850 

Added lines #L847 - L850 were not covered by tests

private onUpdateCustomData() {
if (this.updateCustomDataHandler) {
this.updateCustomDataHandler();
Expand Down
1 change: 1 addition & 0 deletions src/renderer/view/main/CustomLayout.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
/>
<RecordPane
v-else-if="c.type === 'Record'"
class="full"
:show-comment="!!c.showCommentColumn"
:show-elapsed-time="!!c.showElapsedTimeColumn"
:show-top-control="!!c.topControlBox"
Expand Down
43 changes: 43 additions & 0 deletions src/renderer/view/main/MobileControls.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<template>
<div>
<div class="full row controls">
<button @click="store.changePly(0)">
<Icon :icon="IconType.FIRST" />
</button>
<button @click="store.goBack()">
<Icon :icon="IconType.BACK" />
</button>
<button @click="store.goForward()">
<Icon :icon="IconType.NEXT" />
</button>
<button @click="store.changePly(Number.MAX_SAFE_INTEGER)">
<Icon :icon="IconType.LAST" />
</button>
<button @click="store.removeCurrentMove()"><Icon :icon="IconType.DELETE" /></button>
<button @click="isMobileMenuVisible = true">Menu</button>
Comment on lines +4 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add accessibility attributes to navigation buttons.

The buttons lack proper accessibility attributes which could make them difficult to use with screen readers.

Apply these improvements:

-      <button @click="store.changePly(0)">
+      <button 
+        @click="store.changePly(0)"
+        aria-label="Go to start position"
+        role="button">
         <Icon :icon="IconType.FIRST" />
       </button>
-      <button @click="store.goBack()">
+      <button 
+        @click="store.goBack()"
+        aria-label="Go to previous move"
+        role="button">
         <Icon :icon="IconType.BACK" />
       </button>
-      <button @click="store.goForward()">
+      <button 
+        @click="store.goForward()"
+        aria-label="Go to next move"
+        role="button">
         <Icon :icon="IconType.NEXT" />
       </button>
-      <button @click="store.changePly(Number.MAX_SAFE_INTEGER)">
+      <button 
+        @click="store.changePly(Number.MAX_SAFE_INTEGER)"
+        aria-label="Go to end position"
+        role="button">
         <Icon :icon="IconType.LAST" />
       </button>
-      <button @click="store.removeCurrentMove()"><Icon :icon="IconType.DELETE" /></button>
+      <button 
+        @click="store.removeCurrentMove()"
+        aria-label="Remove current move"
+        role="button">
+        <Icon :icon="IconType.DELETE" />
+      </button>
-      <button @click="isMobileMenuVisible = true">Menu</button>
+      <button 
+        @click="isMobileMenuVisible = true"
+        aria-label="Open menu"
+        role="button">
+        {{ $t('menu.title') }}
+      </button>
📝 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.

Suggested change
<button @click="store.changePly(0)">
<Icon :icon="IconType.FIRST" />
</button>
<button @click="store.goBack()">
<Icon :icon="IconType.BACK" />
</button>
<button @click="store.goForward()">
<Icon :icon="IconType.NEXT" />
</button>
<button @click="store.changePly(Number.MAX_SAFE_INTEGER)">
<Icon :icon="IconType.LAST" />
</button>
<button @click="store.removeCurrentMove()"><Icon :icon="IconType.DELETE" /></button>
<button @click="isMobileMenuVisible = true">Menu</button>
<button
@click="store.changePly(0)"
aria-label="Go to start position"
role="button">
<Icon :icon="IconType.FIRST" />
</button>
<button
@click="store.goBack()"
aria-label="Go to previous move"
role="button">
<Icon :icon="IconType.BACK" />
</button>
<button
@click="store.goForward()"
aria-label="Go to next move"
role="button">
<Icon :icon="IconType.NEXT" />
</button>
<button
@click="store.changePly(Number.MAX_SAFE_INTEGER)"
aria-label="Go to end position"
role="button">
<Icon :icon="IconType.LAST" />
</button>
<button
@click="store.removeCurrentMove()"
aria-label="Remove current move"
role="button">
<Icon :icon="IconType.DELETE" />
</button>
<button
@click="isMobileMenuVisible = true"
aria-label="Open menu"
role="button">
{{ $t('menu.title') }}
</button>

</div>
Comment on lines +3 to +18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance mobile touch interaction and semantic structure.

The controls section could benefit from improved mobile interaction and semantic structure:

  1. Add touch event handling for better mobile experience
  2. Use semantic HTML elements for navigation controls
-    <div class="full row controls">
+    <nav class="full row controls" role="navigation" aria-label="Game controls">
-      <button @click="store.changePly(0)">
+      <button 
+        @click="store.changePly(0)"
+        @touchstart.prevent="store.changePly(0)">
         <Icon :icon="IconType.FIRST" />
       </button>
-      <button @click="store.goBack()">
+      <button 
+        @click="store.goBack()"
+        @touchstart.prevent="store.goBack()">
         <Icon :icon="IconType.BACK" />
       </button>
       <!-- Similar changes for other buttons -->
-    </div>
+    </nav>

Committable suggestion was skipped due to low confidence.

<FileMenu v-if="isMobileMenuVisible" @close="isMobileMenuVisible = false" />
Comment on lines +17 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance menu button and dialog implementation.

The menu button and dialog implementation could be improved for better mobile UX and internationalization:

-      <button @click="isMobileMenuVisible = true">Menu</button>
+      <button 
+        @click="isMobileMenuVisible = true"
+        class="menu-button">
+        {{ $t('menu.title') }}
+      </button>
-    <FileMenu v-if="isMobileMenuVisible" @close="isMobileMenuVisible = false" />
+    <FileMenu 
+      v-if="isMobileMenuVisible" 
+      @close="isMobileMenuVisible = false"
+      class="mobile-menu" />

Add these styles:

.menu-button {
  font-weight: 500;
}
.menu-button:active {
  background-color: rgba(0, 0, 0, 0.1);
}
.mobile-menu {
  position: fixed;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
  z-index: 1000;
}

</div>
</template>

<script setup lang="ts">
import { IconType } from "@/renderer/assets/icons";
import { useStore } from "@/renderer/store";
import Icon from "@/renderer/view/primitive/Icon.vue";
import FileMenu from "@/renderer/view/menu/FileMenu.vue";
import { ref } from "vue";

const store = useStore();
const isMobileMenuVisible = ref(false);
</script>
Comment on lines +23 to +32
Copy link

@coderabbitai coderabbitai bot Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding cleanup for menu visibility state.

The menu visibility state should be reset when the component is unmounted to prevent potential issues if the component is re-mounted.

Add cleanup using onUnmounted:

 import { IconType } from "@/renderer/assets/icons";
 import { useStore } from "@/renderer/store";
 import Icon from "@/renderer/view/primitive/Icon.vue";
 import FileMenu from "@/renderer/view/menu/FileMenu.vue";
-import { ref } from "vue";
+import { ref, onUnmounted } from "vue";

 const store = useStore();
 const isMobileMenuVisible = ref(false);
+
+onUnmounted(() => {
+  isMobileMenuVisible.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.

Suggested change
<script setup lang="ts">
import { IconType } from "@/renderer/assets/icons";
import { useStore } from "@/renderer/store";
import Icon from "@/renderer/view/primitive/Icon.vue";
import FileMenu from "@/renderer/view/menu/FileMenu.vue";
import { ref } from "vue";
const store = useStore();
const isMobileMenuVisible = ref(false);
</script>
<script setup lang="ts">
import { IconType } from "@/renderer/assets/icons";
import { useStore } from "@/renderer/store";
import Icon from "@/renderer/view/primitive/Icon.vue";
import FileMenu from "@/renderer/view/menu/FileMenu.vue";
import { ref, onUnmounted } from "vue";
const store = useStore();
const isMobileMenuVisible = ref(false);
onUnmounted(() => {
isMobileMenuVisible.value = false;
});
</script>

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It is internal of script-setup tag.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunfish-shogi, thank you for the clarification. You're absolutely right; since isMobileMenuVisible is scoped within the <script setup> block, it is automatically cleaned up when the component is unmounted. There's no need to reset it manually in onUnmounted. I apologize for the confusion.

(_/)
( •_•)
(>💡<) Reflecting and learning!

✏️ Learnings added 
Learnt from: sunfish-shogi
PR: sunfish-shogi/electron-shogi#982
File: src/renderer/view/main/MobileControls.vue:23-32
Timestamp: 2024-10-25T15:44:42.265Z
Learning: In Vue 3's `<script setup>` context, reactive variables declared are automatically cleaned up upon component unmounting, so manual cleanup in `onUnmounted` is unnecessary.

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.

Comment on lines +23 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance type safety and store usage.

Consider improving type safety and store usage patterns:

 <script setup lang="ts">
 import { IconType } from "@/renderer/assets/icons";
-import { useStore } from "@/renderer/store";
+import type { Store } from "@/renderer/store";
+import { useStore } from "@/renderer/store";
 import Icon from "@/renderer/view/primitive/Icon.vue";
 import FileMenu from "@/renderer/view/menu/FileMenu.vue";
 import { ref } from "vue";
 
-const store = useStore();
+const store: Store = useStore();
 const isMobileMenuVisible = ref<boolean>(false);

Committable suggestion was skipped due to low confidence.


<style scoped>
.controls button {
font-size: 100%;
width: 100%;
height: 100%;
}
.controls button .icon {
height: 68%;
}
</style>
Comment on lines +34 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance mobile-specific styles.

The current styles could be improved for better mobile interaction:

 <style scoped>
 .controls button {
   font-size: 100%;
   width: 100%;
   height: 100%;
+  min-height: 44px; /* Ensure minimum tap target size */
+  touch-action: manipulation; /* Disable double-tap zoom */
+  transition: transform 0.1s ease;
 }
+
+.controls button:active {
+  transform: scale(0.95);
+  background-color: rgba(0, 0, 0, 0.1);
+}
+
 .controls button .icon {
   height: 68%;
+  pointer-events: none; /* Prevent touch event issues on icons */
 }
 </style>
📝 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.

Suggested change
<style scoped>
.controls button {
font-size: 100%;
width: 100%;
height: 100%;
}
.controls button .icon {
height: 68%;
}
</style>
<style scoped>
.controls button {
font-size: 100%;
width: 100%;
height: 100%;
min-height: 44px; /* Ensure minimum tap target size */
touch-action: manipulation; /* Disable double-tap zoom */
transition: transform 0.1s ease;
}
.controls button:active {
transform: scale(0.95);
background-color: rgba(0, 0, 0, 0.1);
}
.controls button .icon {
height: 68%;
pointer-events: none; /* Prevent touch event issues on icons */
}
</style>

126 changes: 76 additions & 50 deletions src/renderer/view/main/MobileLayout.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,67 +4,84 @@
<div class="column">
<BoardPane
:max-size="boardPaneMaxSize"
:layout-type="BoardLayoutType.PORTRAIT"
:layout-type="boardLayoutType"
@resize="onBoardPaneResize"
/>
<div class="row controls" :style="{ height: `${controlPaneHeight}px` }">
<button @click="store.changePly(0)">
<Icon :icon="IconType.FIRST" />
</button>
<button @click="store.goBack()">
<Icon :icon="IconType.BACK" />
</button>
<button @click="store.goForward()">
<Icon :icon="IconType.NEXT" />
</button>
<button @click="store.changePly(Number.MAX_SAFE_INTEGER)">
<Icon :icon="IconType.LAST" />
</button>
<button @click="store.removeCurrentMove()"><Icon :icon="IconType.DELETE" /></button>
<button @click="isMobileMenuVisible = true">Menu</button>
</div>
<RecordPane
<MobileControls
v-if="showRecordViewOnBottom"
class="auto"
:style="{ width: `${windowSize.width}px`, height: `${bottomRecordViewSize.height}px` }"
:style="{ height: `${controlPaneHeight}px` }"
/>
<RecordPane
v-if="showRecordViewOnBottom && !commentEditorMode"
:style="{
width: `${windowSize.width}px`,
height: `${bottomRecordViewSize.height - toggleHeight}px`,
}"
Comment on lines +16 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure non-negative height in RecordPane style

Subtracting toggleHeight from bottomRecordViewSize.height might result in a negative height, which can cause layout issues. To prevent this, ensure the height is non-negative by using Math.max.

Apply this diff to fix the issue:

16        :style="{
17          width: `${windowSize.width}px`,
-18          height: `${bottomRecordViewSize.height - toggleHeight}px`,
+18          height: `${Math.max(0, bottomRecordViewSize.height - toggleHeight)}px`,
19        }"
📝 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.

Suggested change
:style="{
width: `${windowSize.width}px`,
height: `${bottomRecordViewSize.height - toggleHeight}px`,
}"
:style="{
width: `${windowSize.width}px`,
height: `${Math.max(0, bottomRecordViewSize.height - toggleHeight)}px`,
}"

:show-top-control="false"
:show-bottom-control="false"
:show-branches="false"
:show-elapsed-time="true"
:show-comment="true"
/>
<RecordComment
v-if="showRecordViewOnBottom && commentEditorMode"
:style="{
width: `${windowSize.width}px`,
height: `${bottomRecordViewSize.height - toggleHeight}px`,
}"
/>
Comment on lines +28 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure non-negative height in RecordComment style

Similar to RecordPane, the height calculation might result in a negative value. Use Math.max to ensure the height is not negative.

Apply this diff to fix the issue:

28        :style="{
29          width: `${windowSize.width}px`,
-30          height: `${bottomRecordViewSize.height - toggleHeight}px`,
+30          height: `${Math.max(0, bottomRecordViewSize.height - toggleHeight)}px`,
31        }"
📝 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.

Suggested change
width: `${windowSize.width}px`,
height: `${bottomRecordViewSize.height - toggleHeight}px`,
}"
/>
width: `${windowSize.width}px`,
height: `${Math.max(0, bottomRecordViewSize.height - toggleHeight)}px`,
}"
/>

<ToggleButton
label="コメント編集モード"
:height="toggleHeight"
:value="commentEditorMode"
@change="
(value) => {
commentEditorMode = value;
}
"
/>
Comment on lines +33 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update commentEditorMode correctly using .value

Since commentEditorMode is a ref, you should access its value using .value. Update the :value binding and the @change handler accordingly.

Apply this diff to fix the issue:

33      <ToggleButton
34        label="コメント編集モード"
35        :height="toggleHeight"
-36        :value="commentEditorMode"
+36        :value="commentEditorMode.value"
37        @change="
38          (value) => {
-39            commentEditorMode = value;
+39            commentEditorMode.value = value;
40          }
41        "
📝 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.

Suggested change
label="コメント編集モード"
:height="toggleHeight"
:value="commentEditorMode"
@change="
(value) => {
commentEditorMode = value;
}
"
/>
label="コメント編集モード"
:height="toggleHeight"
:value="commentEditorMode.value"
@change="
(value) => {
commentEditorMode.value = value;
}
"
/>

Comment on lines +35 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix ref access in ToggleButton component.

The commentEditorMode is a ref but it's not being accessed correctly in the template. When using refs in template expressions, Vue automatically unwraps them, but in event handlers, you need to explicitly use .value.

Apply this diff to fix the ref access:

  <ToggleButton
    label="コメント編集モード"
    :height="toggleHeight"
    :value="commentEditorMode"
    @change="
      (value) => {
-       commentEditorMode = value;
+       commentEditorMode.value = value;
      }
    "
  />
📝 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.

Suggested change
:value="commentEditorMode"
@change="
(value) => {
commentEditorMode = value;
}
"
/>
:value="commentEditorMode"
@change="
(value) => {
commentEditorMode.value = value;
}
"
/>

</div>
<div
v-if="!showRecordViewOnBottom"
class="column"
:style="{ width: `${windowSize.width - boardPaneSize.width}px` }"
>
<MobileControls :style="{ height: `${controlPaneHeight}px` }" />
<RecordPane
:style="{ height: `${(windowSize.height - controlPaneHeight) * 0.6}px` }"
:show-top-control="false"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure non-negative height in RecordPane style

The height calculation might result in a negative value if window dimensions are small. Use Math.max to prevent negative heights.

Apply this diff to fix the issue:

-51        :style="{ height: `${(windowSize.height - controlPaneHeight) * 0.6}px` }"
+51        :style="{ height: `${Math.max(0, (windowSize.height - controlPaneHeight) * 0.6)}px` }"

Committable suggestion was skipped due to low confidence.

:show-bottom-control="false"
:show-elapsed-time="true"
:show-comment="true"
/>
<RecordComment
:style="{
'margin-top': '5px',
height: `${(windowSize.height - controlPaneHeight) * 0.4 - 5}px`,
}"
Comment on lines +59 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure non-negative height in RecordComment style

Subtracting values may lead to negative heights. Ensure the height remains non-negative with Math.max.

Apply this diff to fix the issue:

59          'margin-top': '5px',
-60          height: `${(windowSize.height - controlPaneHeight) * 0.4 - 5}px`,
+60          height: `${Math.max(0, (windowSize.height - controlPaneHeight) * 0.4 - 5)}px`,
📝 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.

Suggested change
height: `${(windowSize.height - controlPaneHeight) * 0.4 - 5}px`,
}"
height: `${Math.max(0, (windowSize.height - controlPaneHeight) * 0.4 - 5)}px`,
}"

/>
</div>
<RecordPane
v-if="showRecordViewOnSide"
:show-top-control="false"
:show-bottom-control="false"
:show-elapsed-time="true"
:show-comment="true"
/>
</div>
<FileMenu v-if="isMobileMenuVisible" @close="isMobileMenuVisible = false" />
</div>
</template>

<script setup lang="ts">
import { RectSize } from "@/common/assets/geometry";
import { BoardLayoutType } from "@/common/settings/layout";
import { IconType } from "@/renderer/assets/icons";
import { Lazy } from "@/renderer/helpers/lazy";
import { useStore } from "@/renderer/store";
import BoardPane from "@/renderer/view/main/BoardPane.vue";
import RecordPane from "@/renderer/view/main/RecordPane.vue";
import FileMenu from "@/renderer/view/menu/FileMenu.vue";
import Icon from "@/renderer/view/primitive/Icon.vue";
import { computed, onMounted, onUnmounted, reactive, ref } from "vue";
import MobileControls from "./MobileControls.vue";
import ToggleButton from "@/renderer/view/primitive/ToggleButton.vue";
import RecordComment from "@/renderer/view/tab/RecordComment.vue";

const lazyUpdateDelay = 80;
const minRecordViewWidth = 150;
const minRecordViewHeight = 100;
const toggleHeight = 24;
const minRecordViewWidth = 300;
const minRecordViewHeight = 200;
Comment on lines +79 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making toggle height responsive.

The toggleHeight is hardcoded to 24 pixels, which might not be optimal for all screen sizes and densities. Consider making it responsive similar to how controlPaneHeight is calculated.

Example implementation:

-const toggleHeight = 24;
+const toggleHeight = computed(() => Math.min(windowSize.height * 0.03, windowSize.width * 0.06));

Committable suggestion was skipped due to low confidence.


const store = useStore();
const isMobileMenuVisible = ref(false);
const windowSize = reactive(new RectSize(window.innerWidth, window.innerHeight));
const commentEditorMode = ref(false);

const windowLazyUpdate = new Lazy();
const updateSize = () => {
Expand All @@ -74,27 +91,36 @@ const updateSize = () => {
}, lazyUpdateDelay);
};

const showRecordViewOnBottom = computed(() => windowSize.height >= windowSize.width);
const controlPaneHeight = computed(() =>
Math.min(windowSize.height * 0.08, windowSize.width * 0.12),
Math.min(windowSize.height * 0.06, windowSize.width * 0.12),
);
const boardPaneMaxSize = computed(() => {
const maxSize = new RectSize(windowSize.width, windowSize.height);
if (showRecordViewOnBottom.value) {
maxSize.height -= controlPaneHeight.value + minRecordViewHeight;
} else {
maxSize.width -= minRecordViewWidth;
}
return maxSize;
});
const boardLayoutType = computed(() => {
if (showRecordViewOnBottom.value) {
return windowSize.width < windowSize.height * 0.5
? BoardLayoutType.PORTRAIT
: BoardLayoutType.COMPACT;
} else {
return windowSize.width < windowSize.height * 1.6
? BoardLayoutType.PORTRAIT
: BoardLayoutType.COMPACT;
}
});

const boardPaneMaxSize = computed(
() => new RectSize(windowSize.width, windowSize.height - controlPaneHeight.value),
);
const boardPaneSize = ref(windowSize);
const onBoardPaneResize = (size: RectSize) => {
boardPaneSize.value = size;
};

const showRecordViewOnSide = computed(() => {
return windowSize.width >= boardPaneSize.value.width + minRecordViewWidth;
});
const showRecordViewOnBottom = computed(() => {
return (
!showRecordViewOnSide.value &&
windowSize.height >= boardPaneSize.value.height + minRecordViewHeight
);
});
const bottomRecordViewSize = computed(() => {
return new RectSize(
windowSize.width,
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/view/main/RecordPane.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<div ref="root" class="full column record-pane">
<div ref="root" class="column record-pane">
<div class="auto record">
<RecordView
:record="store.record"
Expand Down