From 0ded27f9df937cb343c7ca83d65044f6f032ce07 Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Thu, 21 May 2026 14:16:58 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20code=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20DRY=20delta-pill,=20var=E2=86=92const/let,=20tokeni?= =?UTF-8?q?ze=20colors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- public/css/shared.css | 2 +- public/js/chart.js | 2 +- public/js/players.js | 47 ++++++++++++++++++------------- views/partials/delta-pill.ejs | 10 +++++++ views/partials/player-history.ejs | 20 +++---------- views/partials/ratings-table.ejs | 18 ++---------- 6 files changed, 47 insertions(+), 52 deletions(-) create mode 100644 views/partials/delta-pill.ejs diff --git a/public/css/shared.css b/public/css/shared.css index c619efc..e8a7d67 100644 --- a/public/css/shared.css +++ b/public/css/shared.css @@ -662,7 +662,7 @@ a:hover { } .delta-pill.flat { - background: oklch(0.95 0.004 260); + background: var(--paper-2); color: var(--ink-3); } diff --git a/public/js/chart.js b/public/js/chart.js index 09cc5d4..970d99a 100644 --- a/public/js/chart.js +++ b/public/js/chart.js @@ -96,7 +96,7 @@ function createRatingChart(container, history) { if (isLast) { svg.appendChild(el('circle', { cx: p.x.toFixed(1), cy: p.y.toFixed(1), - r: '4', fill: 'var(--accent)', stroke: 'white', 'stroke-width': '2' + r: '4', fill: 'var(--accent)', stroke: 'var(--paper)', 'stroke-width': '2' })); } else { svg.appendChild(el('circle', { diff --git a/public/js/players.js b/public/js/players.js index e622de3..bea9bc1 100644 --- a/public/js/players.js +++ b/public/js/players.js @@ -1,6 +1,17 @@ -var cachedDebugInfo = {}; -var pendingPlayerData = null; -var openPdgaNumber = null; +const cachedDebugInfo = {}; +let pendingPlayerData = null; +let openPdgaNumber = null; + +// ── Delta-pill helper ───────────────────────────── +// Returns { text, cls, html } — use .text/.cls to update existing DOM elements +// safely; .html is available for insertion contexts. +function renderDeltaPill(value, extraClass) { + if (value == null) return null; + const cls = value > 0 ? 'up' : value < 0 ? 'down' : 'flat'; + const text = value > 0 ? '+' + value : String(value); + const extra = extraClass ? ' ' + extraClass : ''; + return { text, cls, html: `${text}` }; +} function setupTooltipsAfterSwap() { document.body.addEventListener('htmx:afterSwap', function(event) { @@ -49,16 +60,16 @@ function initRatingsTooltips() { } function togglePlayerHistory(pdgaNumber) { - var historyRow = document.getElementById('history-' + pdgaNumber); - var contentDiv = document.getElementById('history-content-' + pdgaNumber); - var expandableRow = document.getElementById('row-' + pdgaNumber); + const historyRow = document.getElementById('history-' + pdgaNumber); + const contentDiv = document.getElementById('history-content-' + pdgaNumber); + const expandableRow = document.getElementById('row-' + pdgaNumber); - var isOpen = historyRow.style.display === 'table-row'; + const isOpen = historyRow.style.display === 'table-row'; // Close any previously-open row if (openPdgaNumber !== null && openPdgaNumber !== pdgaNumber) { - var prevHistory = document.getElementById('history-' + openPdgaNumber); - var prevRow = document.getElementById('row-' + openPdgaNumber); + const prevHistory = document.getElementById('history-' + openPdgaNumber); + const prevRow = document.getElementById('row-' + openPdgaNumber); if (prevHistory) { prevHistory.style.display = 'none'; prevHistory.classList.remove('is-open'); @@ -144,11 +155,9 @@ async function refreshPlayer(pdgaNumber) { const deltaMonthPill = ratingCell.querySelector('.delta-pill'); if (deltaMonthPill && data.player.ratingChange != null) { - const pillChange = data.player.ratingChange; - const pillText = pillChange > 0 ? `+${pillChange}` : pillChange.toString(); - const pillClass = pillChange > 0 ? 'up' : pillChange < 0 ? 'down' : 'flat'; - deltaMonthPill.textContent = pillText; - deltaMonthPill.className = `delta-pill ${pillClass}`; + const pill = renderDeltaPill(data.player.ratingChange); + deltaMonthPill.textContent = pill.text; + deltaMonthPill.className = `delta-pill ${pill.cls}`; } } } catch (error) { @@ -509,15 +518,15 @@ function closeAddPlayerModal(event) { // ── Sparkline toggle ─────────────────────────────── document.addEventListener('DOMContentLoaded', function() { - var btn = document.getElementById('trendchart-toggle'); + const btn = document.getElementById('trendchart-toggle'); if (!btn) return; - var state = localStorage.getItem('ratingtracker.sparklines') || 'on'; + const state = localStorage.getItem('ratingtracker.sparklines') || 'on'; document.body.dataset.sparklines = state; btn.setAttribute('aria-pressed', state === 'on' ? 'true' : 'false'); btn.addEventListener('click', function() { - var next = document.body.dataset.sparklines === 'on' ? 'off' : 'on'; + const next = document.body.dataset.sparklines === 'on' ? 'off' : 'on'; document.body.dataset.sparklines = next; btn.setAttribute('aria-pressed', next === 'on' ? 'true' : 'false'); localStorage.setItem('ratingtracker.sparklines', next); @@ -527,9 +536,9 @@ document.addEventListener('DOMContentLoaded', function() { // ── Expandable row keyboard support ─────────────── document.addEventListener('keydown', function(e) { if (e.key !== 'Enter' && e.key !== ' ') return; - var row = e.target; + const row = e.target; if (!row.classList || !row.classList.contains('expandable-row')) return; e.preventDefault(); - var pdgaNumber = row.id.replace('row-', ''); + const pdgaNumber = row.id.replace('row-', ''); togglePlayerHistory(parseInt(pdgaNumber, 10)); }); diff --git a/views/partials/delta-pill.ejs b/views/partials/delta-pill.ejs new file mode 100644 index 0000000..819e94e --- /dev/null +++ b/views/partials/delta-pill.ejs @@ -0,0 +1,10 @@ +<%/* delta-pill.ejs — renders a Δ-pill span. + Locals: + value {number|null} — the delta value + extraClass {string} — optional additional CSS class (e.g. 'delta-predicted-pill') +*/%> +<% if (typeof value !== 'undefined' && value != null) { + const _cls = value > 0 ? 'up' : value < 0 ? 'down' : 'flat'; + const _text = value > 0 ? '+' + value : value.toString(); + const _xtra = (typeof extraClass !== 'undefined' && extraClass) ? ' ' + extraClass : ''; +%><%= _text %><% } %> diff --git a/views/partials/player-history.ejs b/views/partials/player-history.ejs index ba627d1..fdbda08 100644 --- a/views/partials/player-history.ejs +++ b/views/partials/player-history.ejs @@ -1,14 +1,6 @@ <% -var monthChange = (typeof player !== 'undefined' && player) ? player.ratingChange : null; -var monthPillText = monthChange != null ? (monthChange > 0 ? '+' + monthChange : monthChange.toString()) : null; -var monthPillClass = monthChange > 0 ? 'up' : monthChange < 0 ? 'down' : 'flat'; - -var gapPredicted = (typeof player !== 'undefined' && player) ? (player.deltaPredicted ?? null) : null; -var gapPillText = gapPredicted != null ? (gapPredicted > 0 ? '+' + gapPredicted : gapPredicted.toString()) : null; -var gapPillClass = gapPredicted > 0 ? 'up' : gapPredicted < 0 ? 'down' : 'flat'; - -var hasPlayer = (typeof player !== 'undefined' && player); -var chartPdgaNumber = hasPlayer ? player.pdgaNumber : pdgaNumber; +const hasPlayer = (typeof player !== 'undefined' && player); +const chartPdgaNumber = hasPlayer ? player.pdgaNumber : pdgaNumber; %>
<% if (hasPlayer) { %> @@ -24,9 +16,7 @@ var chartPdgaNumber = hasPlayer ? player.pdgaNumber : pdgaNumber;
Change vs last month
- <% if (monthPillText) { %> - <%= monthPillText %> - <% } else { %>—<% } %> + <% if (player.ratingChange != null) { %><%- include('delta-pill', { value: player.ratingChange }) %><% } else { %>—<% } %>
@@ -36,9 +26,7 @@ var chartPdgaNumber = hasPlayer ? player.pdgaNumber : pdgaNumber;
Gap to predicted
- <% if (gapPillText) { %> - <%= gapPillText %> - <% } else { %>—<% } %> + <% if (player.deltaPredicted != null) { %><%- include('delta-pill', { value: player.deltaPredicted, extraClass: 'delta-predicted-pill' }) %><% } else { %>—<% } %>
diff --git a/views/partials/ratings-table.ejs b/views/partials/ratings-table.ejs index d6bf061..e8df398 100644 --- a/views/partials/ratings-table.ejs +++ b/views/partials/ratings-table.ejs @@ -43,15 +43,7 @@ function renderSparkline(values) { <% ratings.forEach(function(player, index) { - var ratingChange = player.ratingChange; - var ratingChangePillText = ratingChange != null ? (ratingChange > 0 ? '+' + ratingChange : ratingChange.toString()) : null; - var ratingChangePillClass = ratingChange > 0 ? 'up' : ratingChange < 0 ? 'down' : 'flat'; - - var deltaPredicted = player.deltaPredicted ?? null; - var deltaPredictedPillText = deltaPredicted != null ? (deltaPredicted > 0 ? '+' + deltaPredicted : deltaPredicted.toString()) : null; - var deltaPredictedPillClass = deltaPredicted > 0 ? 'up' : deltaPredicted < 0 ? 'down' : 'flat'; - - var sparklineSvg = renderSparkline(player.monthlyHistory || []); + const sparklineSvg = renderSparkline(player.monthlyHistory || []); %> <%= index + 1 %> @@ -65,9 +57,7 @@ function renderSparkline(values) { <%- player.rating || 'Click refresh' %>
- <% if (ratingChangePillText) { %> - <%= ratingChangePillText %> - <% } %> + <%- include('delta-pill', { value: player.ratingChange }) %> <% if (sparklineSvg) { %> <%- sparklineSvg %> <% } %> @@ -79,9 +69,7 @@ function renderSparkline(values) {
- <% if (deltaPredictedPillText) { %> - <%= deltaPredictedPillText %> - <% } %> + <%- include('delta-pill', { value: player.deltaPredicted, extraClass: 'delta-predicted-pill' }) %>