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

Add customAttribution option to AttributionControl #7033

Merged
merged 4 commits into from
Jul 27, 2018

Conversation

mklopets
Copy link
Contributor

@mklopets mklopets commented Jul 26, 2018

Adds a way to add custom attribution strings to AttributionControl. Also exposes the customAttribution option on Map.

This implements the feature request in #1485.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks good! A few nitpicks.

@@ -111,6 +113,13 @@ class AttributionControl {
_updateAttributions() {
if (!this._map.style) return;
let attributions: Array<string> = [];
if (this.options.customAttribution) {
if (this.options.customAttribution instanceof Array) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Array.isArray instead — instanceof Array can sometimes fail.

src/ui/map.js Outdated
@@ -61,6 +61,7 @@ type MapOptions = {
container: HTMLElement | string,
bearingSnap?: number,
attributionControl?: boolean,
customAttribution?: string | string[],
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Array<string> form for consistency.

@mklopets
Copy link
Contributor Author

btw, I noticed that running build locally adds "scheme"?: "xyz" | "tms" to VectorSourceSpecification in flow-typed/style-spec.js – is this intentionally not committed in?

@mourner
Copy link
Member

mourner commented Jul 27, 2018

@mklopets not intentionally, don't worry about it — we'll commit.

@@ -16,6 +17,7 @@ type Options = {
* @implements {IControl}
* @param {Object} [options]
* @param {boolean} [options.compact] If `true` force a compact attribution that shows the full attribution on mouse hover, or if `false` force the full attribution control. The default is a responsive attribution that collapses when the map is less than 640 pixels wide.
* @param {string | Array<string>} [options.customAttribution] String or strings to show ahead of any other attributions.
Copy link
Collaborator

@andrewharvey andrewharvey Jul 27, 2018

Choose a reason for hiding this comment

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

It might just be me, but I think after would be easier to understand in this context than ahead.

Copy link
Contributor Author

@mklopets mklopets Jul 27, 2018

Choose a reason for hiding this comment

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

Actually, this JSDoc is incorrect, everything is sorted by length anyway – I'll change the doc. Thanks!

@mourner mourner merged commit 08ef570 into mapbox:master Jul 27, 2018
@mklopets mklopets deleted the custom-attributions branch July 27, 2018 14:48
@mklopets
Copy link
Contributor Author

@mourner could we get this released? Would be much appreciated – needed in production.

@mourner
Copy link
Member

mourner commented Jul 30, 2018

@mklopets unfortunately not at the moment — we're on a monthly release cadence so you should expect the new version in about 3 weeks. However, you can always build the master version and use it in production.

@mklopets
Copy link
Contributor Author

Oh, alright :/ I tried looking up the release cadence from the docs here in the repo but couldn't find anything. Might be good to add a little note to CONTRIBUTING.md about it.

Thanks for the quick response(s)!

mklopets added a commit to mklopets/DefinitelyTyped that referenced this pull request Aug 23, 2018
mklopets added a commit to mklopets/DefinitelyTyped that referenced this pull request Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants