#2877 - Group Genmapper Colors

[kodinkat]

• fixes: Group Genmapper colors #2877

---

[kodinkat]

@corsacca review ready....

[corsacca]

Thanks @kodinkat!

[github-actions]

Code Review

Good PR overall — the WCAG-based text contrast logic is a real improvement and the refactoring direction is correct. A few issues worth addressing:

---

1. Incomplete refactoring: enhanceNodeData still has duplicate status color logic

The new ensureStatusColor() helper was created to eliminate the repeated inline status color resolution blocks, but the IIFE inside enhanceNodeData was not updated to use it:

// genmap-tile.js — enhanceNodeData (not changed in this PR)
statusColor:
  node.statusColor ||
  (() => {
    const colors = window.dtGroupGenmap?.statusField?.colors || {};
    const archivedKey = window.dtGroupGenmap?.statusField?.archived_key || '';
    if (node.status && colors[node.status]) {
      return colors[node.status];
    } else if (node.status && archivedKey && node.status === archivedKey) {
      return '#808080';
    }
    return getStatusColor(node.status);
  })(),

This IIFE is effectively the same logic as ensureStatusColor(). Since enhanceNodeData is the primary path for building node data, this should be the canonical place to call the new helper. The inconsistency means a future bug fix in one place won't be reflected in the other.

---

2. Redundant ensureStatusColor call in the stroke attribute callback

In both render paths, the stroke attribute callback calls ensureStatusColor(d.data) purely for the side effect, but the return value isn't used:

.attr('fill', (d) => ensureStatusColor(d.data) || '#3f729b')  // sets statusColor on data
.attr('stroke', (d) => {
  ensureStatusColor(d.data);  // redundant — fill already ran and set it
  return d.data.status === ...
})

Since D3 processes chained .attr() calls in order, statusColor is guaranteed to be set by the time stroke runs. The ensureStatusColor call in stroke can be dropped.

---

3. CSS fill removed from .node text — full reliance on JS

Removing fill: #333 from .group-genmap-chart .node text is intentional (it allows JS to control color dynamically), but it creates a fragile dependency: if JS fails to set the fill attribute for any reason, SVG text will have no explicit fill and will inherit from the document root (typically black, but browser-dependent and potentially invisible on dark node backgrounds).

Keeping a CSS fallback on the base .node text rule as a default would be safer. The JS attr('fill', ...) call will override CSS attr-level fill just fine. Note that .node-generation still has fill: #ffffff in CSS — inconsistent with .node-title now having no CSS fallback.

---

4. WCAG luminance threshold is non-standard (acceptable, but worth documenting)

const textColor = luminance < 0.5 ? '#ffffff' : '#333333';

The comment says this was intentionally lowered from the standard WCAG threshold. The true WCAG AA threshold for contrast ratio 4.5:1 with white text puts the crossover luminance at ~0.178, and for 3:1 at ~0.3. Using 0.5 ensures white text on a wider range of backgrounds but may produce white text on backgrounds that would pass with dark text at lower contrast. The comment explains the intent well enough, but it's worth noting this isn't a strict WCAG calculation — the function JSDoc says "Uses WCAG contrast guidelines" which slightly overstates it.

---

Minor

• console.warn in getTextColorForBackground is fine during development. Worth ensuring this doesn't fire on valid production input (e.g., 3-character hex like #fff would currently trigger it — those are valid CSS colors). If the status colors from dtGroupGenmap could ever be short hex, the validator would need to handle that case.
• The d3 global added to eslint.config.mjs is correct and appropriate.
• Using NODE_FONT_SIZE + 'px' (string concatenation) instead of a template literal is a minor style inconsistency with the rest of the file, but not a real issue.

---

Summary

The core feature (dynamic text contrast based on background luminance) is well-implemented. Issues 1 and 2 are the most actionable — completing the ensureStatusColor refactor to cover enhanceNodeData and removing the redundant call in stroke would make the cleanup complete.