feat: allow docking the floating button to the left or right side#1076
feat: allow docking the floating button to the left or right side#1076Sufyr wants to merge 5 commits intomengxi-ream:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 8857d56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
the
size:XL
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: Read Frog - Open Source Immersive Translate Configuration Schema and MigrationView Suggested Changes@@ -33,7 +33,7 @@
**Note:** The schema version and last modified time are tracked in the configuration's metadata (`meta`), not as fields inside the config object itself. This improves separation of user settings and system metadata, and is reflected in all import/export and sync operations.
-**Current schema version:** The latest `CONFIG_SCHEMA_VERSION` is **59**.
+**Current schema version:** The latest `CONFIG_SCHEMA_VERSION` is **60**.
#### Configuration Initialization Optimization
The `initializeConfig()` function optimizes storage operations by tracking changes during initialization:
@@ -538,6 +538,48 @@
- This change provides users with a convenient way to disable hover translation without removing their hotkey preference
This migration was introduced in PR #1068 to give users more control over the hover translation feature directly from the options page.
+
+#### Example: v59 to v60 Migration (Floating Button Anchor)
+The migration from v59 to v60 adds an `anchor` field to the `floatingButton` configuration, allowing users to choose whether the floating button docks to the left or right side of the screen.
+
+**Key changes:**
+- Adds `anchor` field (type: enum `"left" | "right"`) to `floatingButton` configuration
+- Default value is `"right"` to maintain backward compatibility with existing configurations
+- Users can configure the anchor side in the options page floating button settings
+- Users can also drag the floating button across the screen to switch between left and right sides
+- The floating button's horizontal position is automatically adjusted based on the anchor side
+
+The migration script adds this field with a default value of `"right"` to all existing configurations:
+
+```ts
+// migration-scripts/v059-to-v060.ts
+export function migrate(oldConfig: any): any {
+ const floatingButton = oldConfig.floatingButton
+
+ if (!floatingButton) {
+ return oldConfig
+ }
+
+ if (floatingButton.anchor) {
+ return oldConfig
+ }
+
+ return {
+ ...oldConfig,
+ floatingButton: {
+ ...floatingButton,
+ anchor: "right",
+ },
+ }
+}
+```
+
+**Implementation details:**
+- The UI includes an anchor selector in the Floating Button settings page
+- Dragging the floating button to the opposite side of the screen automatically updates the anchor setting
+- The `anchor` field works alongside the existing `position` field (vertical position) to fully determine the button's placement
+
+This migration provides users with greater control over floating button placement, accommodating both left-handed and right-handed preferences.
#### Example: v57 to v58 Migration (Subtitle Position Persistence)
The migration from v57 to v58 adds a `position` field to the `videoSubtitles` configuration to persist the subtitle overlay's drag position across page navigations.
@@ -930,6 +972,26 @@
}
```
+#### v60 Example (Floating Button Anchor)
+```ts
+export const testSeries = {
+ 'complex-config-from-v020': {
+ description: 'Adds anchor field to floatingButton config',
+ config: {
+ // ... other config fields
+ floatingButton: {
+ enabled: true,
+ position: 0.75,
+ anchor: 'right', // New field added in v060
+ disabledFloatingButtonPatterns: ['github.com'],
+ clickAction: 'panel',
+ },
+ // ...
+ },
+ },
+}
+```
+
#### v58 Example (Subtitle Position Persistence)
```ts
export const testSeries = {Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/wise-sides-dock.md">
<violation number="1" location=".changeset/wise-sides-dock.md:2">
P2: Changeset frontmatter is missing required `extensionVersion` minimum (1.15.0), which can mis-target extension update notifications.</violation>
</file>
<file name="src/utils/config/migration-scripts/v058-to-v059.ts">
<violation number="1" location="src/utils/config/migration-scripts/v058-to-v059.ts:9">
P2: Migration script can throw on invalid persisted config by dereferencing `oldConfig` before null/object checks.</violation>
</file>
<file name="src/entrypoints/side.content/components/floating-button/index.tsx">
<violation number="1" location="src/entrypoints/side.content/components/floating-button/index.tsx:78">
P2: Global drag listeners and `userSelect` lock are not lifecycle-cleaned on unmount/interruption; teardown relies only on `mouseup`.</violation>
<violation number="2" location="src/entrypoints/side.content/components/floating-button/index.tsx:97">
P2: Drag Y clamping fails when viewport is shorter than min+offset, allowing invalid (including negative) persisted button positions.</violation>
</file>
<file name="src/entrypoints/options/pages/floating-button/floating-button-anchor.tsx">
<violation number="1" location="src/entrypoints/options/pages/floating-button/floating-button-anchor.tsx:26">
P2: Using a spread of render-time `floatingButton` when writing atom state can overwrite newer sibling fields; update only the changed key to avoid lost updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@read-frog/extension": patch | |||
There was a problem hiding this comment.
P2: Changeset frontmatter is missing required extensionVersion minimum (1.15.0), which can mis-target extension update notifications.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/wise-sides-dock.md, line 2:
<comment>Changeset frontmatter is missing required `extensionVersion` minimum (1.15.0), which can mis-target extension update notifications.</comment>
<file context>
@@ -0,0 +1,5 @@
+---
+"@read-frog/extension": patch
+---
+
</file context>
src/entrypoints/side.content/components/floating-button/index.tsx
Outdated
Show resolved
Hide resolved
src/entrypoints/options/pages/floating-button/floating-button-anchor.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/entrypoints/side.content/components/floating-button/index.tsx">
<violation number="1" location="src/entrypoints/side.content/components/floating-button/index.tsx:64">
P2: Drag cleanup does not always reset `isDraggingButton`, which can leave the floating button stuck hidden/non-interactive.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/entrypoints/side.content/components/floating-button/index.tsx
Outdated
Show resolved
Hide resolved
Code Review — PR #1076OverallSolid feature addition. The dock-side setting, drag-to-switch, and z-index fix are well-scoped. Migration, localization, and options UI all follow existing patterns. A few items worth addressing: Issues1. Left-anchor doesn't offset for side panel When 2. Drag position can exceed viewport on resize Position is stored as a ratio ( 3.
4. useEffect(() => {
if (isDraggingButton) return
if (dragPosition !== null && dragAnchor !== null) {
void setFloatingButton({ position: dragPosition, anchor: dragAnchor })
setDragPosition(null)
setDragAnchor(null)
}
if (dragPreview !== null) {
setDragPreview(null)
}
}, [dragAnchor, dragPosition, dragPreview, isDraggingButton, setFloatingButton])This effect fires when 5.
Positives
Suggestions (non-blocking)
Verdict: Approve with minor suggestions. The core implementation is sound. The drag refactor is cleaner than the original, the migration is correct, and the UI adapts well for both sides. The effect dependency on |
PR Review: feat: allow docking the floating button to the left or right sideOverall AssessmentTaste Score: Good taste Well-structured PR that adds left/right docking with drag-to-switch. Clean migration, good i18n coverage (8 locales), proper schema update. The drag rewrite fixes real lifecycle bugs from the original code. Positive Highlights
Issues and Suggestions1. Bug risk: Left-anchor + side panel openWhen the button is on the left side and the side panel is open, the button stays at left: 0px. The side panel opens on the right, so left-docked should be fine, but worth verifying there is no layout shift edge case where the button overlaps content. 2. clamp helperMath.clamp is not standard JS yet, so the custom clamp function is necessary. Consider checking if a shared utility already exists in the codebase to avoid duplication. 3. Drag preview does not account for side panel widthThe anchor detection correctly considers the side panel via availableWidth. However, the drag preview x clamp uses full window.innerWidth, so the preview could visually appear behind the side panel. Minor visual issue, not a blocker. 4. Schema: anchor is .optional().default("right")This means existing configs without anchor get the default via Zod parsing. Combined with the migration script, there is double-coverage. This is fine -- belt and suspenders approach for data integrity. 5. Small viewport edge caseOn viewports shorter than ~230px, window.innerHeight - FLOATING_BUTTON_MAX_TOP_OFFSET could be less than FLOATING_BUTTON_MIN_TOP, locking the button to a single vertical position. The Math.max guard handles this correctly. Acceptable edge case. SecurityNo concerns. Changes are purely UI/config with no external data handling. PerformanceNo concerns. The drag handler properly cleans up listeners. The pointer-events-none opacity-0 on the real button during drag (while showing a lightweight preview) avoids layout thrashing. Test Coverage
SummaryThis is a well-executed feature PR. The drag lifecycle rewrite is a clear improvement over the original. Migration is correct. i18n is comprehensive. The code is readable and follows existing patterns. A few minor edge cases noted above but nothing blocking. Recommendation: Approve |
Sufyr
force-pushed
the
feat/floating-button-dock-side
branch
2 times, most recently
from
130ff3e to
7af1f55
Compare
PR Review: feat: allow docking the floating button to the left or right sideOverallSolid feature addition. Clean migration, good i18n coverage across all 8 locales, and the drag rework is a meaningful improvement. A few items worth addressing: Code QualityGood:
Suggestions:
Potential Bugs
Performance
SecurityNo concerns. No user input is used unsanitized, no new message passing or external data flows introduced. Test Coverage
Minor Nits
SummaryThe feature works well with a clean implementation. The main actionable items are #5 (verify partial update doesn't lose fields) and #9 (add basic unit tests for pure logic). Everything else is polish. |
PR Review: feat: allow docking the floating button to the left or right sideOverall this is a well-structured PR. The refactoring from Potential Bugs1. Migration script uses truthiness check instead of value validation if (floatingButton.anchor) {
return oldConfig
}If if (floatingButton.anchor === "left" || floatingButton.anchor === "right") {
return oldConfig
}2. The dropdown was changed to non-modal. This affects accessibility behavior (focus trapping, backdrop overlay, Escape key handling). If intentional, worth documenting in the PR description. If not intentional, this could degrade keyboard/screen-reader UX. Code Quality3. The drag preview uses 4. const attachSideClassName = isDraggingButton || isSideOpen || isDropdownOpen ? "translate-x-0" : ""On the left side, the hidden state is 5. Left-anchor positioning is hardcoded to Test Coverage6. No tests for drag-to-switch-sides logic
Minor / Nits7. Changeset uses 8. Chinese comments 9. Defensive if (value !== "left" && value !== "right") { return }
What looks good
Verdict: Solid PR. The main actionable items are the migration truthiness check (#1), confirming the |
| ) | ||
| const newPosition = newY / window.innerHeight | ||
| setDragPosition(newPosition) | ||
| const cleanupDragListeners = useCallback(() => { |
There was a problem hiding this comment.
cleanupDragListeners is wrapped in useCallback with [setIsDraggingButton] dep, setIsDraggingButton is from Jotai useAtom, it is not stable and can cause unnecessary cleanupDragListener call.
Recommend to do cleanup directly in handleMouseUp and unmount
There was a problem hiding this comment.
Updated in a new commit. Could you check if this resolves the issue?
iykon
left a comment
iykon
left a comment
There was a problem hiding this comment.
the rest of the changes LGTM.
can you check the conflicts?
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 950f9fec37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| y: clamp( | ||
| moveEvent.clientY, | ||
| DRAG_PREVIEW_SIZE / 2, | ||
| window.innerHeight - DRAG_PREVIEW_SIZE / 2, |
There was a problem hiding this comment.
The drag preview Y position is clamped to the full viewport (DRAG_PREVIEW_SIZE / 2 to window.innerHeight - DRAG_PREVIEW_SIZE / 2), but the persisted button position is clamped much higher (FLOATING_BUTTON_MIN_TOP to window.innerHeight - FLOATING_BUTTON_MAX_TOP_OFFSET). When users drag near the bottom, the preview shows the button can be dropped there, then it snaps upward on mouseup, which makes placement feel inconsistent. Clamp preview Y with the same min/max used for newPosition so the preview reflects the actual drop result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@Sufyr can you check this? I can reproduce locally with the change
There was a problem hiding this comment.
@Sufyr can you check this? I can reproduce locally with the change
i think the current behavior is fine, need to restrict the draggable height to the final placement range?
There was a problem hiding this comment.
oh ok, it is consistent to current behaviour
Sufyr
force-pushed
the
feat/floating-button-dock-side
branch
from
950f9fe to
8857d56
Compare
conflicts are resolved now |
|
感谢一直的改进，我过几天会看看这个 PR |
github-actions
bot
added
the
contrib-trust:trusted
Contributor trust score23/100 — New contributor This score estimates contributor familiarity with Outcome
Score breakdown
Signals used
Policy
Updated automatically when the PR changes or when a maintainer reruns the workflow. |
github-actions
bot
removed
the
contrib-trust:trusted
github-actions
bot
added
contrib-trust:moderate
3 participants
Type of Changes
Description
Related Issue
Closes #1038
How Has This Been Tested?
Screenshots
Checklist
Additional Information
Summary by cubic
Adds a Dock Side setting for the floating button so users can pin it to the left or right, switch sides by dragging, and see a drag preview. Polishes drag lifecycle, bounds, and menu layering for a smoother, predictable UI.
New Features
floatingButton.anchor. Existing configs auto-migrate to right.Bug Fixes
Written for commit 8857d56. Summary will update on new commits.