From 355cdd9f07a39b041c6016657b62ed3ab71c6662 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 8 Feb 2024 12:57:08 +0100 Subject: [PATCH] fix: browser inBbox setting not persistent and features click not opening popup Refactor the way it updates itself in the process. --- umap/static/umap/base.css | 5 + umap/static/umap/js/umap.browser.js | 132 +++++++++++++------------ umap/static/umap/js/umap.layer.js | 6 +- umap/tests/integration/test_browser.py | 44 +++++++++ 4 files changed, 123 insertions(+), 64 deletions(-) diff --git a/umap/static/umap/base.css b/umap/static/umap/base.css index c9a9b83f..bb86cc79 100644 --- a/umap/static/umap/base.css +++ b/umap/static/umap/base.css @@ -679,6 +679,9 @@ input[type=hidden].blur + [type="button"] { overflow-x: hidden; } #umap-ui-container { + /* Added for playwright to consider the element as non visible */ + /* as being out of the visible viewport is not enough */ + visibility: hidden; position: absolute; bottom: 0; padding: 0 10px; @@ -970,6 +973,7 @@ input:invalid { } .umap-ui #umap-ui-container { right: 0; + visibility: visible; } } @media all and (orientation:portrait) { @@ -982,6 +986,7 @@ input:invalid { } .umap-ui #umap-ui-container { right: 0; + visibility: visible; } .umap-ui .leaflet-right { right: 0; diff --git a/umap/static/umap/js/umap.browser.js b/umap/static/umap/js/umap.browser.js index a43b0e4f..6267c9cd 100644 --- a/umap/static/umap/js/umap.browser.js +++ b/umap/static/umap/js/umap.browser.js @@ -6,9 +6,13 @@ L.U.Browser = L.Class.extend({ initialize: function (map) { this.map = map + this.map.on('moveend', this.onMoveEnd, this) }, - addFeature: function (feature) { + addFeature: function (feature, parent) { + const filter = this.options.filter + if (filter && !feature.matchFilter(filter, this.filterKeys)) return + if (this.options.inBbox && !feature.isOnScreen(this.bounds)) return const feature_li = L.DomUtil.create('li', `${feature.getClassName()} feature`), zoom_to = L.DomUtil.create('i', 'feature-zoom_to', feature_li), edit = L.DomUtil.create('i', 'show-on-edit feature-edit', feature_li), @@ -48,58 +52,76 @@ L.U.Browser = L.Class.extend({ ) L.DomEvent.on(edit, 'click', feature.edit, feature) L.DomEvent.on(del, 'click', feature.confirmDelete, feature) - return feature_li + // HOTFIX. Remove when this is released: + // https://github.com/Leaflet/Leaflet/pull/9052 + L.DomEvent.disableClickPropagation(feature_li) + parent.appendChild(feature_li) }, - addDatalayer: function (datalayer, dataContainer) { - const filterKeys = this.map.getFilterKeys() - const container = L.DomUtil.create( - 'div', - datalayer.getHidableClass(), - dataContainer - ), - headline = L.DomUtil.create('h5', '', container) - container.id = `browse_data_datalayer_${datalayer.umap_id}` + datalayerId: function (datalayer) { + return `browse_data_datalayer_${L.stamp(datalayer)}` + }, + + onDataLayerChanged: function (e) { + this.updateDatalayer(e.target) + }, + + addDataLayer: function (datalayer, parent) { + const container = L.DomUtil.create('div', datalayer.getHidableClass(), parent), + headline = L.DomUtil.create('h5', '', container), + counter = L.DomUtil.create('span', 'datalayer-counter', headline) + container.id = this.datalayerId(datalayer) datalayer.renderToolbox(headline) L.DomUtil.add('span', '', headline, datalayer.options.name) const ul = L.DomUtil.create('ul', '', container) - L.DomUtil.classIf(container, 'off', !datalayer.isVisible()) - - const build = () => { - ul.innerHTML = '' - const bounds = this.map.getBounds() - datalayer.eachFeature((feature) => { - if ( - this.options.filter && - !feature.matchFilter(this.options.filter, filterKeys) - ) - return - if (this.options.inBbox && !feature.isOnScreen(bounds)) return - ul.appendChild(this.addFeature(feature)) - }) - } - - build() - let total = datalayer.count(), - current = ul.querySelectorAll('li').length, - count = total == current ? total : `${current}/${total}` - const counter = L.DomUtil.add('span', 'datalayer-counter', headline, count) - counter.title = L._('Features in this layer: {count}', {count: count}) - datalayer.on('datachanged', build) - datalayer.map.ui.once('panel:closed', () => { - datalayer.off('datachanged', build) - this.map.off('moveend', build) + this.updateDatalayer(datalayer) + datalayer.on('datachanged', this.onDataLayerChanged, this) + this.map.ui.once('panel:closed', () => { + datalayer.off('datachanged', this.onDataLayerChanged, this) }) - datalayer.map.ui.once('panel:ready', () => { - datalayer.map.ui.once('panel:ready', () => { - datalayer.off('datachanged', build) - }) + }, + + updateDatalayer: function (datalayer) { + // Compute once, but use it for each feature later. + this.bounds = this.map.getBounds() + const parent = L.DomUtil.get(this.datalayerId(datalayer)) + // Panel is not open + if (!parent) return + L.DomUtil.classIf(parent, 'off', !datalayer.isVisible()) + const container = parent.querySelector('ul'), + counter = parent.querySelector('.datalayer-counter') + container.innerHTML = '' + datalayer.eachFeature((feature) => this.addFeature(feature, container)) + + let total = datalayer.count(), + current = container.querySelectorAll('li').length, + count = total == current ? total : `${current}/${total}` + counter.textContent = count + counter.title = L._('Features in this layer: {count}', { count: count }) + }, + + onFormChange: function () { + this.map.eachBrowsableDataLayer((datalayer) => { + datalayer.resetLayer(true) + this.updateDatalayer(datalayer) + }) + }, + + onMoveEnd: function () { + const isBrowserOpen = !!document.querySelector('.umap-browse-data') + if (!isBrowserOpen) return + const isListDynamic = this.options.inBbox + this.map.eachBrowsableDataLayer((datalayer) => { + if (!isListDynamic && !datalayer.hasDynamicData()) return + this.updateDatalayer(datalayer) }) }, open: function () { + // Get once but use it for each feature later + this.filterKeys = this.map.getFilterKeys() const container = L.DomUtil.create('div', 'umap-browse-data') - // HOTFIX. Remove when this is merged and released: + // HOTFIX. Remove when this is released: // https://github.com/Leaflet/Leaflet/pull/9052 L.DomEvent.disableClickPropagation(container) @@ -113,39 +135,23 @@ L.U.Browser = L.Class.extend({ const formContainer = L.DomUtil.create('div', '', container) const dataContainer = L.DomUtil.create('div', 'umap-browse-features', container) - const rebuildHTML = () => { - dataContainer.innerHTML = '' - this.map.eachBrowsableDataLayer((datalayer) => { - this.addDatalayer(datalayer, dataContainer) - }) - } - const redrawDataLayers = () => { - this.map.eachBrowsableDataLayer((datalayer) => { - datalayer.resetLayer(true) - }) - } const fields = [ ['options.filter', { handler: 'Input', placeholder: L._('Filter') }], ['options.inBbox', { handler: 'Switch', label: L._('Current map view') }], ] const builder = new L.U.FormBuilder(this, fields, { makeDirty: false, - callback: (e) => { - if (e.helper.field === 'options.inBbox') { - if (this.options.inBbox) this.map.on('moveend', rebuildHTML) - else this.map.off('moveend', rebuildHTML) - } - redrawDataLayers() - rebuildHTML() - }, + callback: () => this.onFormChange(), }) formContainer.appendChild(builder.build()) - rebuildHTML() - this.map.ui.openPanel({ data: { html: container }, actions: [this.map._aboutLink()], }) + + this.map.eachBrowsableDataLayer((datalayer) => { + this.addDataLayer(datalayer, dataContainer) + }) }, }) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 1b7f8b06..b0ea74ec 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -743,9 +743,13 @@ L.U.DataLayer = L.Evented.extend({ return !((!isNaN(from) && zoom < from) || (!isNaN(to) && zoom > to)) }, + hasDynamicData: function () { + return !!(this.options.remoteData && this.options.remoteData.dynamic) + }, + fetchRemoteData: async function (force) { if (!this.isRemoteLayer()) return - if (!this.options.remoteData.dynamic && this.hasDataLoaded() && !force) return + if (!this.hasDynamicData() && this.hasDataLoaded() && !force) return if (!this.isVisible()) return let url = this.map.localizeUrl(this.options.remoteData.url) if (this.options.remoteData.proxy) { diff --git a/umap/tests/integration/test_browser.py b/umap/tests/integration/test_browser.py index c665f71f..bc1a0282 100644 --- a/umap/tests/integration/test_browser.py +++ b/umap/tests/integration/test_browser.py @@ -153,6 +153,50 @@ def test_data_browser_bbox_limit_should_be_dynamic(live_server, page, bootstrap, expect(page.get_by_text("one line in new zeland")).to_be_hidden() +def test_data_browser_bbox_filter_should_be_persistent( + live_server, page, bootstrap, map +): + # Zoom on Europe + page.goto(f"{live_server.url}{map.get_absolute_url()}#6/51.000/2.000") + el = page.get_by_text("Current map view") + expect(el).to_be_visible() + el.click() + browser = page.locator("#umap-ui-container") + expect(browser.get_by_text("one point in france")).to_be_visible() + expect(browser.get_by_text("one line in new zeland")).to_be_hidden() + expect(browser.get_by_text("one polygon in greenland")).to_be_hidden() + # Close and reopen the browser to make sure this settings is persistent + close = browser.get_by_text("Close") + close.click() + expect(browser).to_be_hidden() + sleep(0.5) + expect(browser.get_by_text("one point in france")).to_be_hidden() + expect(browser.get_by_text("one line in new zeland")).to_be_hidden() + expect(browser.get_by_text("one polygon in greenland")).to_be_hidden() + page.get_by_title("See data layers").click() + page.get_by_role("button", name="Browse data").click() + expect(browser.get_by_text("one point in france")).to_be_visible() + expect(browser.get_by_text("one line in new zeland")).to_be_hidden() + expect(browser.get_by_text("one polygon in greenland")).to_be_hidden() + + +def test_data_browser_bbox_filtered_is_clickable(live_server, page, bootstrap, map): + popup = page.locator(".leaflet-popup") + # Zoom on Europe + page.goto(f"{live_server.url}{map.get_absolute_url()}#6/51.000/2.000") + el = page.get_by_text("Current map view") + expect(el).to_be_visible() + el.click() + browser = page.locator("#umap-ui-container") + expect(browser.get_by_text("one point in france")).to_be_visible() + expect(browser.get_by_text("one line in new zeland")).to_be_hidden() + expect(browser.get_by_text("one polygon in greenland")).to_be_hidden() + expect(popup).to_be_hidden() + browser.get_by_text("one point in france").click() + expect(popup).to_be_visible() + expect(popup.get_by_text("one point in france")).to_be_visible() + + def test_data_browser_with_variable_in_name(live_server, page, bootstrap, map): # Include a variable map.settings["properties"]["labelKey"] = "{name} ({foo})"