Skip to content
This repository has been archived by the owner on Oct 28, 2020. It is now read-only.

[Fix bug 944758] Provide attribution to OpenStreetMap data. #557

Merged
merged 1 commit into from Dec 18, 2013

Conversation

johngian
Copy link
Contributor

@johngian johngian commented Dec 2, 2013

No description provided.

@@ -173,3 +173,9 @@ function paginatorSelector(selector) {
window.location = $(this).val();
});
}

function addAttributionOSM(map) {
var osmURL = 'http://openstreetmap.org';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add this to a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that just to keep the line length small. It can be replaced in the attribution string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Point on keeping the line small but maybe just splitting the line in multiple lines is better than assigning strings to variables?

@alexgibson
Copy link
Member

As a note to this PR, I believe we also need to add an attribution on pages where we use the static maps API?

https://www.mapbox.com/developers/api/#Static.API

@johngian
Copy link
Contributor Author

johngian commented Dec 4, 2013

I dont have in mind a straightforward way to add a leaflet-like OSM contribution in static tiles.
Maybe we can add the attribution in the page footer or below the map image, when needed.

Thoughts?

@alexgibson
Copy link
Member

I don't think there's a way to add an attribution directly to a static map, so yes adding something to the page should be enough I think.


{% block endrow %}
<div class="end-row row attribution">
<p class="grayed">
Copy link
Member

Choose a reason for hiding this comment

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

To make the attribution text line up with the grid, add the CSS classes of large-12 columns to the <p> element.

Also it is good to try and avoid using presentational class names (what if gray turns to blue in a future design?)

In CSS/LESS you could apply the .grayed class using a selector such as:

.attribution p {
    .grayed;
}

@johngian
Copy link
Contributor Author

johngian commented Dec 5, 2013

@alexgibson PR updated

@alexgibson
Copy link
Member

Looking good, r+ from me on the attribution styling

@johngian
Copy link
Contributor Author

@glogiotatidis r?

{% block endrow %}
<div class="end-row row attribution">
<p class="large-12 columns">
Maps &copy; by <a href="http://mapbox.com">Mapbox</a> and <a href="http://openstreetmap.org/copyright">OpenStreetMap</a> contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

If is doesn't cause formating errors I'd prefer this line smaller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also since this blocked is share, it should be in separate file and included here and in view_report.html and everywhere else needed.

@alexgibson
Copy link
Member

Since #562 was merged, do we also still need to add an attribution to the new report template?

@glogiotatidis
Copy link
Contributor

r+wc

@johngian
Copy link
Contributor Author

@alexgibson I added the same endrow contribution in the new reports pages.

@alexgibson
Copy link
Member

r+ 🚀

alexgibson added a commit that referenced this pull request Dec 18, 2013
[Fix bug 944758] Provide attribution to OpenStreetMap data.
@alexgibson alexgibson merged commit 9ac3f34 into mozilla:master Dec 18, 2013
@johngian johngian deleted the 944758-osm-attribution branch December 18, 2013 14:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants