From 355cdd9f07a39b041c6016657b62ed3ab71c6662 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 8 Feb 2024 12:57:08 +0100 Subject: [PATCH 1/2] 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})" From 4cec24797e90de654b174114b2521a9987d2e9d8 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 8 Feb 2024 13:09:18 +0100 Subject: [PATCH 2/2] chore: move Browser to a module --- .../{umap.browser.js => modules/browser.js} | 96 ++++++++++--------- umap/static/umap/js/modules/global.js | 3 +- umap/static/umap/js/modules/request.js | 2 +- umap/static/umap/js/umap.js | 2 +- umap/static/umap/test/index.html | 1 - 5 files changed, 53 insertions(+), 51 deletions(-) rename umap/static/umap/js/{umap.browser.js => modules/browser.js} (66%) diff --git a/umap/static/umap/js/umap.browser.js b/umap/static/umap/js/modules/browser.js similarity index 66% rename from umap/static/umap/js/umap.browser.js rename to umap/static/umap/js/modules/browser.js index 6267c9cd..a4e7fedb 100644 --- a/umap/static/umap/js/umap.browser.js +++ b/umap/static/umap/js/modules/browser.js @@ -1,24 +1,26 @@ -L.U.Browser = L.Class.extend({ - options: { - filter: '', - inBbox: false, - }, +// Uses `L._`` from Leaflet.i18n which we cannot import as a module yet +import { DomUtil, DomEvent } from '../../vendors/leaflet/leaflet-src.esm.js' - initialize: function (map) { +export default class Browser { + constructor(map) { this.map = map this.map.on('moveend', this.onMoveEnd, this) - }, + this.options = { + filter: '', + inBbox: false, + } + } - addFeature: function (feature, parent) { + addFeature(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), - del = L.DomUtil.create('i', 'show-on-edit feature-delete', feature_li), - colorBox = L.DomUtil.create('i', 'feature-color', feature_li), - title = L.DomUtil.create('span', 'feature-title', feature_li), + const feature_li = DomUtil.create('li', `${feature.getClassName()} feature`), + zoom_to = DomUtil.create('i', 'feature-zoom_to', feature_li), + edit = DomUtil.create('i', 'show-on-edit feature-edit', feature_li), + del = DomUtil.create('i', 'show-on-edit feature-delete', feature_li), + colorBox = DomUtil.create('i', 'feature-color', feature_li), + title = DomUtil.create('span', 'feature-title', feature_li), symbol = feature._getIconUrl ? L.U.Icon.prototype.formatUrl(feature._getIconUrl(), feature) : null @@ -32,7 +34,7 @@ L.U.Browser = L.Class.extend({ const icon = L.U.Icon.makeIconElement(symbol, colorBox) L.U.Icon.setIconContrast(icon, colorBox, symbol, bgcolor) } - L.DomEvent.on( + DomEvent.on( zoom_to, 'click', function (e) { @@ -41,7 +43,7 @@ L.U.Browser = L.Class.extend({ }, feature ) - L.DomEvent.on( + DomEvent.on( title, 'click', function (e) { @@ -50,44 +52,44 @@ L.U.Browser = L.Class.extend({ }, feature ) - L.DomEvent.on(edit, 'click', feature.edit, feature) - L.DomEvent.on(del, 'click', feature.confirmDelete, feature) + DomEvent.on(edit, 'click', feature.edit, feature) + DomEvent.on(del, 'click', feature.confirmDelete, feature) // HOTFIX. Remove when this is released: // https://github.com/Leaflet/Leaflet/pull/9052 - L.DomEvent.disableClickPropagation(feature_li) + DomEvent.disableClickPropagation(feature_li) parent.appendChild(feature_li) - }, + } - datalayerId: function (datalayer) { + datalayerId(datalayer) { return `browse_data_datalayer_${L.stamp(datalayer)}` - }, + } - onDataLayerChanged: function (e) { + onDataLayerChanged(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) + addDataLayer(datalayer, parent) { + const container = DomUtil.create('div', datalayer.getHidableClass(), parent), + headline = DomUtil.create('h5', '', container), + counter = 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) + DomUtil.add('span', '', headline, datalayer.options.name) + const ul = DomUtil.create('ul', '', container) this.updateDatalayer(datalayer) datalayer.on('datachanged', this.onDataLayerChanged, this) this.map.ui.once('panel:closed', () => { datalayer.off('datachanged', this.onDataLayerChanged, this) }) - }, + } - updateDatalayer: function (datalayer) { + updateDatalayer(datalayer) { // Compute once, but use it for each feature later. this.bounds = this.map.getBounds() - const parent = L.DomUtil.get(this.datalayerId(datalayer)) + const parent = DomUtil.get(this.datalayerId(datalayer)) // Panel is not open if (!parent) return - L.DomUtil.classIf(parent, 'off', !datalayer.isVisible()) + DomUtil.classIf(parent, 'off', !datalayer.isVisible()) const container = parent.querySelector('ul'), counter = parent.querySelector('.datalayer-counter') container.innerHTML = '' @@ -98,16 +100,16 @@ L.U.Browser = L.Class.extend({ count = total == current ? total : `${current}/${total}` counter.textContent = count counter.title = L._('Features in this layer: {count}', { count: count }) - }, + } - onFormChange: function () { + onFormChange() { this.map.eachBrowsableDataLayer((datalayer) => { datalayer.resetLayer(true) this.updateDatalayer(datalayer) }) - }, + } - onMoveEnd: function () { + onMoveEnd() { const isBrowserOpen = !!document.querySelector('.umap-browse-data') if (!isBrowserOpen) return const isListDynamic = this.options.inBbox @@ -115,25 +117,25 @@ L.U.Browser = L.Class.extend({ if (!isListDynamic && !datalayer.hasDynamicData()) return this.updateDatalayer(datalayer) }) - }, + } - open: function () { + open() { // Get once but use it for each feature later this.filterKeys = this.map.getFilterKeys() - const container = L.DomUtil.create('div', 'umap-browse-data') + const container = DomUtil.create('div', 'umap-browse-data') // HOTFIX. Remove when this is released: // https://github.com/Leaflet/Leaflet/pull/9052 - L.DomEvent.disableClickPropagation(container) + DomEvent.disableClickPropagation(container) - const title = L.DomUtil.add( + const title = DomUtil.add( 'h3', 'umap-browse-title', container, this.map.options.name ) - const formContainer = L.DomUtil.create('div', '', container) - const dataContainer = L.DomUtil.create('div', 'umap-browse-features', container) + const formContainer = DomUtil.create('div', '', container) + const dataContainer = DomUtil.create('div', 'umap-browse-features', container) const fields = [ ['options.filter', { handler: 'Input', placeholder: L._('Filter') }], @@ -153,5 +155,5 @@ L.U.Browser = L.Class.extend({ this.map.eachBrowsableDataLayer((datalayer) => { this.addDataLayer(datalayer, dataContainer) }) - }, -}) + } +} diff --git a/umap/static/umap/js/modules/global.js b/umap/static/umap/js/modules/global.js index 381b5497..d73356d4 100644 --- a/umap/static/umap/js/modules/global.js +++ b/umap/static/umap/js/modules/global.js @@ -1,9 +1,10 @@ import * as L from '../../vendors/leaflet/leaflet-src.esm.js' import URLs from './urls.js' +import Browser from './browser.js' import { Request, ServerRequest, RequestError, HTTPError, NOKError } from './request.js' // Import modules and export them to the global scope. // For the not yet module-compatible JS out there. // Copy the leaflet module, it's expected by leaflet plugins to be writeable. window.L = { ...L } -window.umap = { URLs, Request, ServerRequest, RequestError, HTTPError, NOKError } +window.umap = { URLs, Request, ServerRequest, RequestError, HTTPError, NOKError, Browser } diff --git a/umap/static/umap/js/modules/request.js b/umap/static/umap/js/modules/request.js index de1ce4b1..b90fbf92 100644 --- a/umap/static/umap/js/modules/request.js +++ b/umap/static/umap/js/modules/request.js @@ -1,5 +1,5 @@ // Uses `L._`` from Leaflet.i18n which we cannot import as a module yet -import { Evented, DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js' +import { DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js' export class RequestError extends Error {} diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index b29d6969..cee3d54d 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -423,7 +423,7 @@ L.U.Map.include({ this._controls.permanentCredit = new L.U.PermanentCreditsControl(this) if (this.options.scrollWheelZoom) this.scrollWheelZoom.enable() else this.scrollWheelZoom.disable() - this.browser = new L.U.Browser(this) + this.browser = new window.umap.Browser(this) this.importer = new L.U.Importer(this) this.drop = new L.U.DropControl(this) this.share = new L.U.Share(this) diff --git a/umap/static/umap/test/index.html b/umap/static/umap/test/index.html index 6935dcbd..23c9d52f 100644 --- a/umap/static/umap/test/index.html +++ b/umap/static/umap/test/index.html @@ -44,7 +44,6 @@ -