Skip to content

enhancement (UI): add draggable resize handle for sidebar width adjustment#62

Open
AmiiirCom wants to merge 12 commits into
sartoopjj:mainfrom
AmiiirCom:main
Open

enhancement (UI): add draggable resize handle for sidebar width adjustment#62
AmiiirCom wants to merge 12 commits into
sartoopjj:mainfrom
AmiiirCom:main

Conversation

@AmiiirCom
Copy link
Copy Markdown

Summary

Adds a draggable resize handle to the sidebar, allowing users to adjust its width dynamically. Improves UI flexibility and user control over layout space.

@sartoopjj
Copy link
Copy Markdown
Owner

AI:

Nice addition — clean, scoped to one file, and the mobile breakpoint disables the handle correctly. A few things to address before merging:

  1. min-width makes the 200px floor unreachable. .sidebar has min-width: 280px (index.html:112), so dragging below 280 has no visual effect and saveSidebarWidth() ends up storing 280 anyway. Either bump the JS clamp to 280 or drop min-width.

  2. Touch listeners need { passive: false }. touchstart / touchmove are passive by default in modern browsers, so the e.preventDefault() calls are ignored and log a console warning. Pass { passive: false } explicitly when registering them.

  3. transition: width 0.05s ease-out on .sidebar fights the drag — causes visible lag pixel-by-pixel. Either remove it, or gate it behind a :not(.dragging) class toggled on mousedown/up.

Minor: the new .sidebar { max-width: none } inside the mobile media query duplicates the existing .sidebar block right below it — worth merging. Also, the two Persian comments are the only non-English comments in the file; up to you whether to harmonize.

Happy to approve once 1–3 are fixed.

@AmiiirCom
Copy link
Copy Markdown
Author

I tried to fix them.

@sartoopjj
Copy link
Copy Markdown
Owner

Hi again

Review written with AI assistance.

Nice small feature. A few things to fix before merge:

1. Default width is now broken. You removed width: 280px from .sidebar but only kept min-width: 280px. Inside the display:flex parent the flex item has no preferred width, so the layout depends on flex defaults. Set a default explicitly and let the JS override via inline style:

.sidebar { width: 280px; min-width: 280px; max-width: 70vw; }

Then drop the duplicate .sidebar { max-width: 70vw } block at the top.

2. RTL inverted. App's primary mode is Persian RTL — sidebar sits on the right visually. Your drag math is newWidth = startWidth + (e.clientX - startX), so dragging right grows the sidebar even though the handle moves into the chat area. Flip when RTL:

var rtl = document.documentElement.dir === 'rtl';
var delta = (e.clientX - startX) * (rtl ? -1 : 1);
var newWidth = startWidth + delta;

3. Global non-passive touchmove listener. You attach it once at IIFE init, even when no drag is happening. Non-passive touchmove blocks the browser's scroll-optimisation path everywhere, not just over the handle. Add it inside touchstart and removeEventListener on touchend, like you already do for the mouse path.

4. !important smell on mobile. Only needed because JS writes style="width:…" inline. If you split that into a class you toggle (.sidebar.user-resized { width: var(--sidebar-w) }) you avoid the !important chain. Not blocking.

5. Width clamping. On window shrink you clamp to 0.7 * innerWidth, but loadSidebarWidth silently refuses out-of-range saved values — combined with point 1 the sidebar can end up with no width at all. Either accept and clamp on load, or always force a default first.

Min must-fix: 1, 2, 3. Thanks for the contribution.

@AmiiirCom
Copy link
Copy Markdown
Author

Thanks for the detailed review. I've gone through each point and made the necessary fixes. Here's the breakdown:

✅ 1 – Default width broken

Fixed. Added width: 280px to .sidebar in CSS and removed the duplicate max-width: 70vw rule. The sidebar now has a proper default width and falls back correctly when no saved width exists.

❌ 2 – RTL drag direction inverted

Not applied (intentional). In this app, the sidebar is physically on the left side in both RTL (Persian) and LTR layouts. The resize handle is always on the left edge of the sidebar. Flipping the delta based on dir="rtl" would make dragging right shrink the sidebar when the text direction is RTL, which feels backwards because the handle is still on the left. To keep the interaction intuitive (drag right → wider sidebar), I left the original unidirectional logic unchanged.

✅ 3 – Global non‑passive touchmove listener

Fixed. Refactored touch dragging: touchmove and touchend listeners are now attached only during an active drag (inside touchstart) and removed immediately after touchend. This eliminates the permanent listener and restores smooth native scrolling. The fix mirrors the mouse event pattern.

⏸️ 4 – !important smell on mobile

Declined (complexity trade‑off). The suggested custom‑property + class approach would add extra state management and complexity. The current inline‑style method works reliably across all targets (WebView, mobile browsers, desktop), and the single !important override is isolated to a mobile media query. I prefer to keep it simple.

✅ 5 – Width clamping and resize handling

Fixed.

  • loadSidebarWidth() now clamps out‑of‑range saved values to [280px, 0.7 * innerWidth] and saves the clamped value.
  • Added a resize event listener that reduces the sidebar width if it exceeds the new maximum and updates storage.

The sidebar now always has a valid width and adapts correctly on window resize.


Summary: Issues 1, 3, and 5 are fully resolved. 2 is kept as‑is for UX consistency. 4 is declined to avoid unnecessary complexity.

Let me know if you'd like any further adjustments.

if (w < 280) w = 280;
if (w > maxW) w = maxW;
sidebar.style.width = w + 'px';
localStorage.setItem('thefeed_sidebar_width', w);
Copy link
Copy Markdown
Contributor

@sepehr-alipour sepehr-alipour May 5, 2026

Choose a reason for hiding this comment

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

This write is redundant on every page load. Only write back if the value actually changed

document.addEventListener('touchmove', onTouchMove, { passive: false });
document.addEventListener('touchend', onTouchEnd);
});
document.addEventListener('touchmove', function(e) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This anonymous touchmove listener duplicates onTouchMove which is already registered inside touchstart. Unlike onTouchMove, this anonymous handler is never removed, it lives on the document forever. Every touch on the page fires it for the lifetime of the app. Remove lines entirely; onTouchMove already handles this.

sidebar.style.width = newWidth + 'px';
e.preventDefault();
}, { passive: false });
document.addEventListener('touchend', function() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same issue as above, this anonymous touchend handler duplicates onTouchEnd and is never cleaned up. Remove lines onTouchEnd covers it.

if (!isDragging) return;
e.preventDefault();
var newWidth = startWidth + (e.clientX - startX);
var maxW = window.innerWidth * 0.7;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

window.innerWidth * 0.7 is calculated in 5+ separate places (load, save, mouseMove, touchMove, resize). Extract to a getMaxWidth() helper to make future changes to the cap easier.

right: 0;
bottom: 0;
width: 100%;
width: 100% !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The !important here is fighting the inline style set by JS.
Cleaner fix: skip calling loadSidebarWidth() on mobile(e.g. guard with if (window.innerWidth <= 768) return;), then no inline style gets set and !important isn't needed

Copy link
Copy Markdown
Contributor

@sepehr-alipour sepehr-alipour left a comment

Choose a reason for hiding this comment

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

The duplicate touch listeners are the only required fix before merge. The rest are polish.

@sartoopjj sartoopjj moved this to In Progress in TheFeed | دفید May 5, 2026
@sartoopjj sartoopjj moved this from In Progress to Todo in TheFeed | دفید May 5, 2026
@sartoopjj
Copy link
Copy Markdown
Owner

any update!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo | برای انجام

Development

Successfully merging this pull request may close these issues.

3 participants