fix: correct sidebar element IDs, add missing CSS classes, and align test mocks
Three-part consistency defect identified in code review:
1. HTML: Four sidebar elements missing CSS class attributes so all CSS
rules targeting .session-sidebar, .sidebar-toggle-btn,
.sidebar-collapse-btn, and .sidebar-list applied to zero elements.
Added class= attributes to all four.
2. JS: initSidebar(), toggleSidebar(), and bindSidebarClickAway() called
$('sidebar') and $('collapse-btn') — neither ID exists in the DOM.
Corrected to $('session-sidebar') and $('sidebar-collapse-btn').
3. Tests: initSidebar/toggleSidebar mocks matched the wrong IDs, masking
the defect. Updated all six mock conditions to match corrected IDs.
4. Coverage gap: test_html_element_classes did not verify new sidebar
elements carry CSS classes. Added four entries for session-sidebar,
sidebar-toggle-btn, sidebar-collapse-btn, and sidebar-list — these
entries would have caught Defect 1 on Task 1's test run.
All 136 JS + 167 Python tests pass. Zero regressions.
This commit is contained in:
@@ -247,6 +247,14 @@ def test_html_element_classes() -> None:
|
|||||||
),
|
),
|
||||||
("session-pill-label", "session-pill__label", "needs max-width truncation"),
|
("session-pill-label", "session-pill__label", "needs max-width truncation"),
|
||||||
("session-pill-bell", "session-pill__bell", "needs amber var(--bell) color"),
|
("session-pill-bell", "session-pill__bell", "needs amber var(--bell) color"),
|
||||||
|
(
|
||||||
|
"session-sidebar",
|
||||||
|
"session-sidebar",
|
||||||
|
"flex-column layout and collapse transition",
|
||||||
|
),
|
||||||
|
("sidebar-toggle-btn", "sidebar-toggle-btn", "36x36 bordered button styles"),
|
||||||
|
("sidebar-collapse-btn", "sidebar-collapse-btn", "chevron button hover styles"),
|
||||||
|
("sidebar-list", "sidebar-list", "flex:1 overflow-y:auto scroll container"),
|
||||||
]
|
]
|
||||||
for el_id, expected_class, reason in cases:
|
for el_id, expected_class, reason in cases:
|
||||||
el = soup.find(id=el_id)
|
el = soup.find(id=el_id)
|
||||||
|
|||||||
+4
-4
@@ -393,7 +393,7 @@ function initSidebar() {
|
|||||||
isOpen = window.innerWidth >= SIDEBAR_NARROW_THRESHOLD;
|
isOpen = window.innerWidth >= SIDEBAR_NARROW_THRESHOLD;
|
||||||
}
|
}
|
||||||
|
|
||||||
const sidebar = $('sidebar');
|
const sidebar = $('session-sidebar');
|
||||||
if (sidebar) {
|
if (sidebar) {
|
||||||
if (isOpen) {
|
if (isOpen) {
|
||||||
sidebar.classList.remove('sidebar--collapsed');
|
sidebar.classList.remove('sidebar--collapsed');
|
||||||
@@ -432,7 +432,7 @@ function toggleSidebar() {
|
|||||||
} catch (_) { /* blocked — ok */ }
|
} catch (_) { /* blocked — ok */ }
|
||||||
|
|
||||||
// Apply class
|
// Apply class
|
||||||
const sidebar = $('sidebar');
|
const sidebar = $('session-sidebar');
|
||||||
if (sidebar) {
|
if (sidebar) {
|
||||||
if (isOpen) {
|
if (isOpen) {
|
||||||
sidebar.classList.remove('sidebar--collapsed');
|
sidebar.classList.remove('sidebar--collapsed');
|
||||||
@@ -442,7 +442,7 @@ function toggleSidebar() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Update collapse button text (‹ when open, › when closed)
|
// Update collapse button text (‹ when open, › when closed)
|
||||||
const collapseBtn = $('collapse-btn');
|
const collapseBtn = $('sidebar-collapse-btn');
|
||||||
if (collapseBtn) {
|
if (collapseBtn) {
|
||||||
collapseBtn.textContent = isOpen ? '\u2039' : '\u203a';
|
collapseBtn.textContent = isOpen ? '\u2039' : '\u203a';
|
||||||
}
|
}
|
||||||
@@ -461,7 +461,7 @@ function bindSidebarClickAway() {
|
|||||||
if (!container) return;
|
if (!container) return;
|
||||||
container.addEventListener('click', () => {
|
container.addEventListener('click', () => {
|
||||||
if (window.innerWidth >= SIDEBAR_NARROW_THRESHOLD) return;
|
if (window.innerWidth >= SIDEBAR_NARROW_THRESHOLD) return;
|
||||||
const sidebar = $('sidebar');
|
const sidebar = $('session-sidebar');
|
||||||
if (!sidebar) return;
|
if (!sidebar) return;
|
||||||
if (sidebar.classList.contains('sidebar--collapsed')) return;
|
if (sidebar.classList.contains('sidebar--collapsed')) return;
|
||||||
sidebar.classList.add('sidebar--collapsed');
|
sidebar.classList.add('sidebar--collapsed');
|
||||||
|
|||||||
+4
-4
@@ -30,17 +30,17 @@
|
|||||||
<div id="view-expanded" class="view hidden">
|
<div id="view-expanded" class="view hidden">
|
||||||
<header class="expanded-header">
|
<header class="expanded-header">
|
||||||
<button id="back-btn" class="back-btn" aria-label="Back">←</button>
|
<button id="back-btn" class="back-btn" aria-label="Back">←</button>
|
||||||
<button id="sidebar-toggle-btn" aria-label="Toggle session list">☰</button>
|
<button id="sidebar-toggle-btn" class="sidebar-toggle-btn" aria-label="Toggle session list">☰</button>
|
||||||
<span id="expanded-session-name" class="expanded-session-name"></span>
|
<span id="expanded-session-name" class="expanded-session-name"></span>
|
||||||
<button id="palette-trigger" class="palette-trigger" aria-label="Open command palette">⌘K</button>
|
<button id="palette-trigger" class="palette-trigger" aria-label="Open command palette">⌘K</button>
|
||||||
</header>
|
</header>
|
||||||
<div class="view-body">
|
<div class="view-body">
|
||||||
<div id="session-sidebar">
|
<div id="session-sidebar" class="session-sidebar">
|
||||||
<div class="sidebar-header">
|
<div class="sidebar-header">
|
||||||
<span class="sidebar-title">Sessions</span>
|
<span class="sidebar-title">Sessions</span>
|
||||||
<button id="sidebar-collapse-btn" aria-label="Collapse session list">‹</button>
|
<button id="sidebar-collapse-btn" class="sidebar-collapse-btn" aria-label="Collapse session list">‹</button>
|
||||||
</div>
|
</div>
|
||||||
<div id="sidebar-list"></div>
|
<div id="sidebar-list" class="sidebar-list"></div>
|
||||||
</div>
|
</div>
|
||||||
<div id="terminal-container" class="terminal-container"></div>
|
<div id="terminal-container" class="terminal-container"></div>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
+12
-12
@@ -1829,8 +1829,8 @@ test('initSidebar defaults to open (removes sidebar--collapsed) on wide screens
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1856,8 +1856,8 @@ test('initSidebar defaults to closed (adds sidebar--collapsed) on narrow screens
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1883,8 +1883,8 @@ test('initSidebar respects stored value true regardless of screen width — even
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1908,8 +1908,8 @@ test('toggleSidebar persists state to localStorage — from true toggles to fals
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1930,8 +1930,8 @@ test('toggleSidebar adds sidebar--collapsed class when closing (from open)', ()
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1952,8 +1952,8 @@ test('toggleSidebar removes sidebar--collapsed class when opening (from closed)
|
|||||||
const mockCollapseBtn = { textContent: '' };
|
const mockCollapseBtn = { textContent: '' };
|
||||||
const origGetById = globalThis.document.getElementById;
|
const origGetById = globalThis.document.getElementById;
|
||||||
globalThis.document.getElementById = (id) => {
|
globalThis.document.getElementById = (id) => {
|
||||||
if (id === 'sidebar') return mockSidebar;
|
if (id === 'session-sidebar') return mockSidebar;
|
||||||
if (id === 'collapse-btn') return mockCollapseBtn;
|
if (id === 'sidebar-collapse-btn') return mockCollapseBtn;
|
||||||
return null;
|
return null;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user