Attribution control #642
Attribution control #642
Conversation
|
I don't know if this is the most optimal way of adding/extending the leaflet attribution control, and I'm not comfortable writing eventual tests. The styling of the control (e.g. icon) can be changed of course, but I think it's looking pretty solid, also no RTL issues. |
|
Looks good to me, thank you! |
| 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>'; |
There was a problem hiding this comment.
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?
| }); | ||
| 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>'); |
There was a problem hiding this comment.
The leaflet object exposes the .getContainer() method to get a handle on their HTML.
| @@ -0,0 +1,110 @@ | |||
| export var AttributionButton = L.Control.extend({ | |||
There was a problem hiding this comment.
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({...There was a problem hiding this comment.
I wasn't able to get that to work, feel free to make the suggested changes.
| .leaflet-container .leaflet-control-attribution { | ||
| background-color: #fff; | ||
| border-radius: 1.5em; | ||
| color: currentColor; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'll take your word for it, it probably has better contrast being light on dark anyways.
There was a problem hiding this comment.
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.
|
@Malvoz would you be ok with (us) removing the extra Leaflet logo before we merge this, since we already have the logo in our attribution. Maybe its not intentional, just an artifact of us having left this PR out of main for so long (sorry about that). We'll get the tests running and merge if you agree. Thanks!
|
|
Working on merging these commits here |
Preview
(colors seen below are outdated, see #642 (comment))
Tooltip on hover:
Default attribution opened (we now have room to spell out "Maps4HTML" (CG) which I think seems a bit more enticing):
Keyboard toggling:
attribution-kbd.mp4