refactor: apply code quality suggestions from task-6-mobile-fab review
Three suggestions from the code review applied:
1. Extract _createSessionInput() factory to eliminate five-line
duplication between showNewSessionInput and showFabSessionInput.
Both functions now call the shared factory. Updated 4 existing
tests to check _createSessionInput's body; added 7 new tests
covering factory behavior and factory usage in each consumer.
2. Fix .new-session-fab:focus-visible outline color from var(--accent)
to var(--bg) — the FAB background is already var(--accent), so
using the same color for the outline created zero contrast.
Added 1 new test to guard this going forward.
3. Add early-return guard in showFabSessionInput against duplicate
overlays: if (document.querySelector('.fab-input-overlay')) return;
Prevents a second overlay being created if called programmatically
while one is already open. Added 1 new test.
532 tests pass (9 new tests added, 523 unchanged).
This commit is contained in:
+15
-7
@@ -1292,13 +1292,24 @@ function updateSessionPill(sessions) {
|
|||||||
* On blur: delayed cleanup (150ms) to allow click handlers.
|
* On blur: delayed cleanup (150ms) to allow click handlers.
|
||||||
* @param {HTMLElement} btn - The button element to replace temporarily.
|
* @param {HTMLElement} btn - The button element to replace temporarily.
|
||||||
*/
|
*/
|
||||||
function showNewSessionInput(btn) {
|
/**
|
||||||
|
* Create a new session name input element with shared base configuration.
|
||||||
|
* Used by both showNewSessionInput (inline) and showFabSessionInput (overlay)
|
||||||
|
* to avoid duplicating the five setup properties.
|
||||||
|
* @returns {HTMLInputElement}
|
||||||
|
*/
|
||||||
|
function _createSessionInput() {
|
||||||
const input = document.createElement('input');
|
const input = document.createElement('input');
|
||||||
input.type = 'text';
|
input.type = 'text';
|
||||||
input.className = 'new-session-input';
|
input.className = 'new-session-input';
|
||||||
input.placeholder = 'Session name\u2026';
|
input.placeholder = 'Session name\u2026';
|
||||||
input.autocomplete = 'off';
|
input.autocomplete = 'off';
|
||||||
input.spellcheck = false;
|
input.spellcheck = false;
|
||||||
|
return input;
|
||||||
|
}
|
||||||
|
|
||||||
|
function showNewSessionInput(btn) {
|
||||||
|
const input = _createSessionInput();
|
||||||
|
|
||||||
function cleanup() {
|
function cleanup() {
|
||||||
if (input.parentNode) input.parentNode.removeChild(input);
|
if (input.parentNode) input.parentNode.removeChild(input);
|
||||||
@@ -1331,17 +1342,14 @@ function showNewSessionInput(btn) {
|
|||||||
* visible on mobile regardless of body/view overflow:hidden constraints.
|
* visible on mobile regardless of body/view overflow:hidden constraints.
|
||||||
*/
|
*/
|
||||||
function showFabSessionInput() {
|
function showFabSessionInput() {
|
||||||
|
if (document.querySelector('.fab-input-overlay')) return;
|
||||||
|
|
||||||
const fab = $('new-session-fab');
|
const fab = $('new-session-fab');
|
||||||
|
|
||||||
const overlay = document.createElement('div');
|
const overlay = document.createElement('div');
|
||||||
overlay.className = 'fab-input-overlay';
|
overlay.className = 'fab-input-overlay';
|
||||||
|
|
||||||
const input = document.createElement('input');
|
const input = _createSessionInput();
|
||||||
input.type = 'text';
|
|
||||||
input.className = 'new-session-input';
|
|
||||||
input.placeholder = 'Session name\u2026';
|
|
||||||
input.autocomplete = 'off';
|
|
||||||
input.spellcheck = false;
|
|
||||||
|
|
||||||
overlay.appendChild(input);
|
overlay.appendChild(input);
|
||||||
|
|
||||||
|
|||||||
@@ -1234,7 +1234,7 @@ body {
|
|||||||
}
|
}
|
||||||
|
|
||||||
.new-session-fab:focus-visible {
|
.new-session-fab:focus-visible {
|
||||||
outline: 2px solid var(--accent);
|
outline: 2px solid var(--bg);
|
||||||
outline-offset: 2px;
|
outline-offset: 2px;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
"""Tests for frontend/style.css — design tokens and dark theme."""
|
"""Tests for frontend/style.css — design tokens and dark theme."""
|
||||||
|
|
||||||
import pathlib
|
import pathlib
|
||||||
|
import re
|
||||||
|
|
||||||
CSS_PATH = pathlib.Path(__file__).parent.parent / "frontend" / "style.css"
|
CSS_PATH = pathlib.Path(__file__).parent.parent / "frontend" / "style.css"
|
||||||
|
|
||||||
@@ -1606,3 +1607,19 @@ def test_css_fab_active_transform() -> None:
|
|||||||
assert ".new-session-fab:active" in css, (
|
assert ".new-session-fab:active" in css, (
|
||||||
"Missing .new-session-fab:active rule in style.css"
|
"Missing .new-session-fab:active rule in style.css"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_css_fab_focus_visible_outline_not_accent() -> None:
|
||||||
|
""".new-session-fab:focus-visible outline must not use var(--accent) — same color as FAB background gives zero contrast."""
|
||||||
|
css = read_css()
|
||||||
|
match = re.search(r"\.new-session-fab:focus-visible\s*\{([^}]*)\}", css)
|
||||||
|
assert match, "Missing .new-session-fab:focus-visible rule in style.css"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "outline" in body, ".new-session-fab:focus-visible must have an outline property"
|
||||||
|
# The FAB background IS var(--accent), so using the same color as outline gives zero visible ring.
|
||||||
|
# Must use var(--bg) or var(--text) for sufficient contrast.
|
||||||
|
assert "var(--accent)" not in body, (
|
||||||
|
".new-session-fab:focus-visible outline must not use var(--accent) — "
|
||||||
|
"the FAB background is already var(--accent), so the outline would be invisible. "
|
||||||
|
"Use var(--bg) or var(--text) for contrast."
|
||||||
|
)
|
||||||
|
|||||||
@@ -1543,58 +1543,58 @@ def test_create_new_session_function_exists() -> None:
|
|||||||
|
|
||||||
|
|
||||||
def test_show_new_session_input_creates_input_with_class() -> None:
|
def test_show_new_session_input_creates_input_with_class() -> None:
|
||||||
"""showNewSessionInput must create an input element with class 'new-session-input'."""
|
"""Input setup must set class 'new-session-input' (via _createSessionInput factory)."""
|
||||||
match = re.search(
|
match = re.search(
|
||||||
r"function showNewSessionInput\s*\(\w+\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
_JS,
|
_JS,
|
||||||
re.DOTALL,
|
re.DOTALL,
|
||||||
)
|
)
|
||||||
assert match, "showNewSessionInput function not found"
|
assert match, "_createSessionInput function not found"
|
||||||
body = match.group(1)
|
body = match.group(1)
|
||||||
assert "new-session-input" in body, (
|
assert "new-session-input" in body, (
|
||||||
"showNewSessionInput must create an input with class 'new-session-input'"
|
"_createSessionInput must set input.className = 'new-session-input'"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_show_new_session_input_sets_placeholder() -> None:
|
def test_show_new_session_input_sets_placeholder() -> None:
|
||||||
"""showNewSessionInput must set placeholder to 'Session name…'."""
|
"""Input setup must set placeholder containing 'Session name' (via _createSessionInput factory)."""
|
||||||
match = re.search(
|
match = re.search(
|
||||||
r"function showNewSessionInput\s*\(\w+\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
_JS,
|
_JS,
|
||||||
re.DOTALL,
|
re.DOTALL,
|
||||||
)
|
)
|
||||||
assert match, "showNewSessionInput function not found"
|
assert match, "_createSessionInput function not found"
|
||||||
body = match.group(1)
|
body = match.group(1)
|
||||||
assert "Session name" in body, (
|
assert "Session name" in body, (
|
||||||
"showNewSessionInput must set placeholder containing 'Session name'"
|
"_createSessionInput must set placeholder containing 'Session name'"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_show_new_session_input_disables_autocomplete() -> None:
|
def test_show_new_session_input_disables_autocomplete() -> None:
|
||||||
"""showNewSessionInput must set autocomplete off on the input."""
|
"""Input setup must set autocomplete off (via _createSessionInput factory)."""
|
||||||
match = re.search(
|
match = re.search(
|
||||||
r"function showNewSessionInput\s*\(\w+\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
_JS,
|
_JS,
|
||||||
re.DOTALL,
|
re.DOTALL,
|
||||||
)
|
)
|
||||||
assert match, "showNewSessionInput function not found"
|
assert match, "_createSessionInput function not found"
|
||||||
body = match.group(1)
|
body = match.group(1)
|
||||||
assert "autocomplete" in body.lower() and "off" in body.lower(), (
|
assert "autocomplete" in body.lower() and "off" in body.lower(), (
|
||||||
"showNewSessionInput must set autocomplete off"
|
"_createSessionInput must set autocomplete off"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_show_new_session_input_disables_spellcheck() -> None:
|
def test_show_new_session_input_disables_spellcheck() -> None:
|
||||||
"""showNewSessionInput must set spellcheck false on the input."""
|
"""Input setup must set spellcheck false (via _createSessionInput factory)."""
|
||||||
match = re.search(
|
match = re.search(
|
||||||
r"function showNewSessionInput\s*\(\w+\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
_JS,
|
_JS,
|
||||||
re.DOTALL,
|
re.DOTALL,
|
||||||
)
|
)
|
||||||
assert match, "showNewSessionInput function not found"
|
assert match, "_createSessionInput function not found"
|
||||||
body = match.group(1)
|
body = match.group(1)
|
||||||
assert "spellcheck" in body.lower() and "false" in body.lower(), (
|
assert "spellcheck" in body.lower() and "false" in body.lower(), (
|
||||||
"showNewSessionInput must set spellcheck false"
|
"_createSessionInput must set spellcheck false"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -1928,3 +1928,120 @@ def test_js_fab_click_calls_show_fab_session_input() -> None:
|
|||||||
"bindStaticEventListeners must call showFabSessionInput for FAB click — "
|
"bindStaticEventListeners must call showFabSessionInput for FAB click — "
|
||||||
"the old showNewSessionInput(fab) inserts into body which is clipped by overflow:hidden"
|
"the old showNewSessionInput(fab) inserts into body which is clipped by overflow:hidden"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_show_fab_session_input_guards_against_duplicate_overlay() -> None:
|
||||||
|
"""showFabSessionInput must guard against creating a second overlay if one already exists."""
|
||||||
|
match = re.search(
|
||||||
|
r"function showFabSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\nasync function |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "showFabSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "fab-input-overlay" in body, (
|
||||||
|
"showFabSessionInput must check for an existing .fab-input-overlay before creating another — "
|
||||||
|
"prevents duplicate overlays if called programmatically while one is already open"
|
||||||
|
)
|
||||||
|
# Guard must be present: querySelector('.fab-input-overlay') with early return
|
||||||
|
assert "querySelector" in body and "return" in body, (
|
||||||
|
"showFabSessionInput must guard with document.querySelector('.fab-input-overlay') return; "
|
||||||
|
"at the top to prevent duplicate overlays"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# ============================================================
|
||||||
|
# _createSessionInput factory (quality refactor — suggestion 1)
|
||||||
|
# ============================================================
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_create_session_input_factory_exists() -> None:
|
||||||
|
"""_createSessionInput factory must exist in app.js to eliminate input setup duplication."""
|
||||||
|
assert "function _createSessionInput" in _JS, (
|
||||||
|
"_createSessionInput must be defined in app.js — "
|
||||||
|
"shared factory eliminates duplicated input setup in showNewSessionInput and showFabSessionInput"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_create_session_input_factory_sets_class() -> None:
|
||||||
|
"""_createSessionInput must return an input with class 'new-session-input'."""
|
||||||
|
match = re.search(
|
||||||
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "_createSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "new-session-input" in body, (
|
||||||
|
"_createSessionInput must set input.className = 'new-session-input'"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_create_session_input_factory_sets_placeholder() -> None:
|
||||||
|
"""_createSessionInput must set placeholder containing 'Session name'."""
|
||||||
|
match = re.search(
|
||||||
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "_createSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "Session name" in body, (
|
||||||
|
"_createSessionInput must set placeholder containing 'Session name'"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_create_session_input_factory_disables_autocomplete() -> None:
|
||||||
|
"""_createSessionInput must set autocomplete off."""
|
||||||
|
match = re.search(
|
||||||
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "_createSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "autocomplete" in body.lower() and "off" in body.lower(), (
|
||||||
|
"_createSessionInput must set autocomplete off"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_create_session_input_factory_disables_spellcheck() -> None:
|
||||||
|
"""_createSessionInput must set spellcheck false."""
|
||||||
|
match = re.search(
|
||||||
|
r"function _createSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "_createSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "spellcheck" in body.lower() and "false" in body.lower(), (
|
||||||
|
"_createSessionInput must set spellcheck false"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_show_new_session_input_uses_factory() -> None:
|
||||||
|
"""showNewSessionInput must call _createSessionInput() instead of duplicating input setup."""
|
||||||
|
match = re.search(
|
||||||
|
r"function showNewSessionInput\s*\(\w+\)\s*\{(.*?)(?=\nasync function |\nfunction |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "showNewSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "_createSessionInput" in body, (
|
||||||
|
"showNewSessionInput must call _createSessionInput() to create the input element"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_js_show_fab_session_input_uses_factory() -> None:
|
||||||
|
"""showFabSessionInput must call _createSessionInput() instead of duplicating input setup."""
|
||||||
|
match = re.search(
|
||||||
|
r"function showFabSessionInput\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\nasync function |\n// )",
|
||||||
|
_JS,
|
||||||
|
re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "showFabSessionInput function not found"
|
||||||
|
body = match.group(1)
|
||||||
|
assert "_createSessionInput" in body, (
|
||||||
|
"showFabSessionInput must call _createSessionInput() to create the input element"
|
||||||
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user