Fix tech map legend not updating with layer toggles#2073
Fix tech map legend not updating with layer toggles#2073tsubasakong wants to merge 5 commits intokoala73:mainfrom
Conversation
…tor-1386-globe-fullscreen-panel
…tor-1386-globe-fullscreen-panel
|
@tsubasakong is attempting to deploy a commit to the Elie Team on Vercel. A member of the Team first needs to authorize it. |
koala73
left a comment
There was a problem hiding this comment.
Hey @tsubasakong — thanks for picking this up! The bug is real and the getLegendItems() extraction is a clean direction. A couple of things need to land before this can merge.
🔴 Blocking
1. updateLegend() inside the RAF hot path — DOM churn at 60fps
render() fires on every pan, zoom, time-range change, and layer toggle. By placing updateLegend() inside its RAF callback, every one of those events now does:
querySelector('.deckgl-legend')on every frame- Rebuilds all SVG strings via
getLegendItems()(includinggetCurrentTheme()+ multiplet()i18next lookups) legend.innerHTML = ...— full subtree destroy + rebuilddocument.createElement()for the CII legend, sets itsinnerHTML,appendChild
For finance, happy, and default variants this is entirely unconditional — same items every time, DOM wiped and rebuilt for nothing. On a 60fps pan lasting 300ms that's 18 full DOM rebuilds.
Fix: Move updateLegend() out of render(). Call it in the four layer-mutation paths instead — the CII toggle handler, setLayers(), enableLayer(), toggleLayer(). Each already calls this.render(), so just add this.updateLegend() before those calls.
Alternatively, the minimal guard:
private updateLegend(): void {
const legend = this.legendEl; // cached ref (see P3 below)
if (!legend) return;
const legendItems = this.getLegendItems();
const key = legendItems.map(i => i.label).join(',') + '|cii:' + this.state.layers.ciiChoropleth;
if (legend.dataset.legendKey === key) return; // skip — nothing changed
legend.dataset.legendKey = key;
// ... rest unchanged
}2. ciiChoroplethLegend recreation makes the toggle handler dead code
updateLegend() wipes legend.innerHTML then creates a new <div id="ciiChoroplethLegend"> on every call. The existing CII toggle handler (line 4247) does querySelector('#ciiChoroplethLegend') + manually sets style.display — but render() fires immediately after, updateLegend() recreates the element with the correct display from this.state.layers.ciiChoropleth, and the toggle handler's mutation is silently overwritten. It's now dead code.
Fix: Create ciiLegendEl once in createLegend() and store it as private ciiLegendEl. In updateLegend(), after the innerHTML wipe, do legend.appendChild(this.ciiLegendEl) (DOM move, not clone) and toggle style.display on it. Remove the querySelector('#ciiChoroplethLegend') block from the toggle handler.
🟡 Should Fix
3. syncMapAfterLayoutChange — remove the RAF call
requestAnimationFrame(sync); // fires ~16ms — CSS transition hasn't started
window.setTimeout(sync, delayMs); // fires at 320ms — after transition ✓The original code only had setTimeout(320), which was deliberate (CSS transition duration). The RAF fires before the container has its final dimensions. Remove it, keep only the timeout.
4. finance / happy / default variants don't filter by active layers
Only tech filters entries via this.state.layers[layer]. The default and happy variants both include aircraft (maps to flights) and datacenter — both toggleable. Toggling those off on non-tech variants still shows stale legend entries. Same bug class this PR is fixing.
5. Tests check source text, not behavior
map-fullscreen-resize.test.mjs regex-matches event-handlers.ts as a string. It doesn't verify that the legend actually updates when a layer is toggled (the PR's stated fix for #1967). Consider a unit test on syncMapAfterLayoutChange with a mock ctx.map, and an integration test that toggles a layer and asserts the legend item count changes.
🔵 Nice to Have
- Cache
legendEl: Storethis.legendElincreateLegend()instead ofquerySelectoron every call. shapesobject: The four SVG generator functions are recreated on everygetLegendItems()call. Move to a module-level constant.
Happy to push fixes on top of your branch if that's easier — just let me know!
|
Hey @tsubasakong — thanks for picking this up! The bug is real and the 🔴 Blocking1.
|
|
@tsubasakong — @koala73 left detailed blocking feedback on Mar 27. To move this forward:
Once those are addressed, the should-fix items (RAF in syncMapAfterLayoutChange, non-tech variant filtering) would be good too. |
Summary
Testing
Fixes #1967