Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribution control #642

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/mapml-viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,12 @@ export class MapViewer extends HTMLElement {
// there is a better configuration than that, but at this moment
// I'm not sure how to approach that issue.
// See https://github.com/Maps4HTML/MapML-Leaflet-Client/issues/24
fadeAnimation: true
fadeAnimation: true,
attributionControl: false
});
this._addToHistory();
// the attribution control is not optional
this._attributionControl = this._map.attributionControl.setPrefix('<a href="https://www.w3.org/community/maps4html/" title="W3C Maps for HTML Community Group">Maps4HTML</a> | <a href="https://leafletjs.com" title="A JS library for interactive maps">Leaflet</a>');
this._attributionControl.getContainer().setAttribute("role","group");
this._attributionControl.getContainer().setAttribute("aria-label","Map data attribution");
this._attributionControl = M.attributionButton().addTo(this._map);

this.setControls(false,false,true);
this._crosshair = M.crosshair().addTo(this._map);
Expand Down
45 changes: 39 additions & 6 deletions src/mapml.css
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,9 @@
.leaflet-tooltip-pane .leaflet-tooltip,
.leaflet-touch .leaflet-control-layers,
.leaflet-touch .leaflet-bar,
.leaflet-popup-tip {
box-shadow: 0 1px 3px rgba(0, 0, 0, 0.2), 0 1px 2px rgb(0, 0, 0, 0.3);
}

/* Increase contrast between the attribution container and the underlying content. */
.leaflet-popup-tip,
.leaflet-container .leaflet-control-attribution {
background-color: rgba(255, 255, 255, 0.95);
box-shadow: rgb(0 0 0 / 30%) 0px 1px 4px -1px;
}

/* Remove opinionated styles of attribution links. */
Expand All @@ -155,6 +151,43 @@
text-decoration: revert;
}


/*
* Attribution.
*/

.leaflet-container .leaflet-control-attribution {
background-color: #fff;
border-radius: 1.5em;
color: currentColor;
Copy link
Member

@ahmadayubi ahmadayubi Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may just be a personal preference but the button may fit the overall theme if it's dark on a light background rather than light on a dark background.

Feel free to resolve this conversation if the dark on a light background doesn't fit well.

Copy link
Member Author

@Malvoz Malvoz Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used dark on light initially, but felt better after trying light on dark. I can post screenshots of dark on light too, and we'll decide then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take your word for it, it probably has better contrast being light on dark anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to dark on light in c6420be. I think it is a better default, and once we've enabled customization (using CSS Parts or Custom properties) the theme color can easily be changed by developers.

margin: 5px;
min-height: 30px;
min-width: 30px;
padding: 0;
}

.leaflet-control-attribution summary {
display: block;
list-style: none;
border-radius: 100%;
position: absolute;
bottom: 0;
left: calc(100% - 30px);
line-height: 0;
width: 30px;
height: 30px;
}

.leaflet-control-attribution summary svg {
border-radius: 100%;
width: inherit;
height: inherit;
}

.mapml-attribution-container {
padding: 5px 35px 5px 10px;
}

/*
* Zoom controls.
*/
Expand Down
110 changes: 110 additions & 0 deletions src/mapml/control/AttributionButton.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
export var AttributionButton = L.Control.extend({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks really clean imo.

One suggestion is if it extends Leaflet's Attribution, you can extend Attribution directly and simply overwrite the functions you need changed.

export var AttributionButton = L.Attribution.extend({...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to get that to work, feel free to make the suggested changes.

options: {
position: 'bottomright',
prefix: '<a href="https://www.w3.org/community/maps4html/">Maps for HTML Community Group</a> <span aria-hidden="true">|</span> <a href="https://leafletjs.com" title="A JS library for interactive maps">Leaflet</a>'
},

initialize: function (options) {
L.Util.setOptions(this, options);
this._attributions = {};
},

onAdd: function (map) {
map.attributionControl = this;
this._container = L.DomUtil.create('details', 'leaflet-control-attribution');
L.DomEvent.disableClickPropagation(this._container);

for (var i in map._layers) {
if (map._layers[i].getAttribution) {
this.addAttribution(map._layers[i].getAttribution());
}
}

this._update();

map.on('layeradd', this._addAttribution, this);

return this._container;
},

onRemove: function (map) {
map.off('layeradd', this._addAttribution, this);
},

_addAttribution: function (ev) {
if (ev.layer.getAttribution) {
this.addAttribution(ev.layer.getAttribution());
ev.layer.once('remove', function () {
this.removeAttribution(ev.layer.getAttribution());
}, this);
}
},

setPrefix: function (prefix) {
this.options.prefix = prefix;
this._update();
return this;
},

addAttribution: function (text) {
if (!text) { return this; }

if (!this._attributions[text]) {
this._attributions[text] = 0;
}
this._attributions[text]++;

this._update();

return this;
},

removeAttribution: function (text) {
if (!text) { return this; }

if (this._attributions[text]) {
this._attributions[text]--;
this._update();
}

return this;
},

_update: function () {
if (!this._map) { return; }

var attribs = [];

for (var i in this._attributions) {
if (this._attributions[i]) {
attribs.push(i);
}
}

var prefixAndAttribs = [];

if (this.options.prefix) {
prefixAndAttribs.push(this.options.prefix);
}
if (attribs.length) {
prefixAndAttribs.push(attribs.join(', '));
}

this._container.innerHTML = '<summary title="Map data attribution"><svg aria-hidden="true" xmlns="http://www.w3.org/2000/svg" height="30px" viewBox="0 0 24 24" width="30px" fill="currentColor"><path d="M0 0h24v24H0z" fill="none"/><path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-6h2v6zm0-8h-2V7h2v2z"/></svg></summary>' + '<div class="mapml-attribution-container">' + prefixAndAttribs.join(' <span aria-hidden="true">|</span> ') + '</div>';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that this is the only major difference between Leaflet's attribution control code and this version; maybe we could achieve the same effect by accessing the attributionControl._container property and performing this statement?


}
});

L.Map.mergeOptions({
attributionControl: true
});

L.Map.addInitHook(function () {
if (this.options.attributionControl) {
new AttributionButton().addTo(this);
}
});

export var attributionButton = function (options) {
return new AttributionButton(options);
};
4 changes: 4 additions & 0 deletions src/mapml/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import { DebugOverlay, debugOverlay} from './layers/DebugLayer';
import { QueryHandler } from './handlers/QueryHandler';
import { ContextMenu } from './handlers/ContextMenu';
import { Util } from './utils/Util';
import { AttributionButton, attributionButton } from './control/AttributionButton';
import { ReloadButton, reloadButton } from './control/ReloadButton';
import { FullscreenButton, fullscreenButton } from './control/FullscreenButton';
import { Crosshair, crosshair } from "./layers/Crosshair";
Expand Down Expand Up @@ -640,6 +641,9 @@ M.mapMlFeatures = mapMlFeatures;
M.MapMLLayerControl = MapMLLayerControl;
M.mapMlLayerControl = mapMlLayerControl;

M.AttributionButton = AttributionButton;
M.attributionButton = attributionButton;

M.ReloadButton = ReloadButton;
M.reloadButton = reloadButton;

Expand Down
2 changes: 1 addition & 1 deletion src/mapml/layers/MapLayer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ export var MapMLLayer = L.Layer.extend({
if (licenseLink) {
licenseTitle = licenseLink.getAttribute('title');
licenseUrl = licenseLink.getAttribute('href');
attText = '<a href="' + licenseUrl + '" title="'+licenseTitle+'">'+licenseTitle+'</a>';
attText = '<a href="' + licenseUrl + '">'+licenseTitle+'</a>';
}
L.setOptions(layer,{attribution:attText});
var legendLink = xml.querySelector('map-link[rel=legend]');
Expand Down
7 changes: 3 additions & 4 deletions src/web-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,12 @@ export class WebMap extends HTMLMapElement {
// there is a better configuration than that, but at this moment
// I'm not sure how to approach that issue.
// See https://github.com/Maps4HTML/MapML-Leaflet-Client/issues/24
fadeAnimation: true
fadeAnimation: true,
attributionControl: false
});
this._addToHistory();
// the attribution control is not optional
this._attributionControl = this._map.attributionControl.setPrefix('<a href="https://www.w3.org/community/maps4html/" title="W3C Maps for HTML Community Group">Maps4HTML</a> | <a href="https://leafletjs.com" title="A JS library for interactive maps">Leaflet</a>');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The leaflet object exposes the .getContainer() method to get a handle on their HTML.

this._attributionControl.getContainer().setAttribute("role","group");
this._attributionControl.getContainer().setAttribute("aria-label","Map data attribution");
this._attributionControl = M.attributionButton().addTo(this._map);

this.setControls(false,false,true);
this._crosshair = M.crosshair().addTo(this._map);
Expand Down
6 changes: 0 additions & 6 deletions test/e2e/core/mapElement.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,4 @@ describe("Playwright Map Element Tests", () => {

await expect(projection).toEqual("OSMTILE");
});
test("Ensure attribution control has role='group' aria-label='Map data attribution'", async () => {
let role = await page.evaluate(`document.querySelector('map')._attributionControl.getContainer().getAttribute('role')`);
await expect(role).toEqual("group");
let arialabel = await page.evaluate(`document.querySelector('map')._attributionControl.getContainer().getAttribute('aria-label')`);
await expect(arialabel).toEqual("Map data attribution");
});
});
8 changes: 0 additions & 8 deletions test/e2e/mapml-viewer/mapml-viewer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,6 @@ describe("Playwright mapml-viewer Element Tests", () => {
afterAll(async function () {
await context.close();
});

test("Ensure attribution control has role='group' aria-label='Map data attribution'", async () => {
let role = await page.evaluate(`document.querySelector('mapml-viewer')._attributionControl.getContainer().getAttribute('role')`);
await expect(role).toEqual("group");
let arialabel = await page.evaluate(`document.querySelector('mapml-viewer')._attributionControl.getContainer().getAttribute('aria-label')`);
await expect(arialabel).toEqual("Map data attribution");
});


test("Initial map element extent", async () => {
const extent = await page.$eval(
Expand Down