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

Allow setting order of custom attribution #11196

Merged
merged 7 commits into from
Nov 10, 2021

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Nov 2, 2021

This PR allows users to set an order for their custom attributions to appear on the map. A user may likely expect and want the order of their custom attribution to correspond to the array set in the map constructor (see: #11191).

To do this, filtering and adding custom attributions occurs after cleaning up source attributions. A part of cleaning up the source attributions involves sorting the attributions by length and then searching and removing duplicate substrings. This change was introduced in PR #2453 in order to remove duplicate substrings in the attribution control as a fix for #1804. This change was then also sorting custom attributions by length.

Before:
Screen Shot 2021-11-01 at 4 54 26 PM

After:
Screen Shot 2021-11-01 at 4 53 26 PM

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Allow users to set order of custom attribution</changelog>

@avpeery avpeery changed the title Allow setting order of custom attributions Allow setting order of custom attribution Nov 2, 2021
@avpeery avpeery marked this pull request as draft November 2, 2021 00:51
@avpeery avpeery linked an issue Nov 2, 2021 that may be closed by this pull request
@avpeery avpeery marked this pull request as ready for review November 2, 2021 02:15
@avpeery avpeery requested a review from arindam1993 November 2, 2021 15:27
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.

This looks good. The only unclear thing I'm wondering about is whether any users depend on the deduplication logic for where a custom attribution and an attribution from a tile source may be partial duplicates — this would no longer work as custom attributions are passed as is.

src/ui/control/attribution_control.js Outdated Show resolved Hide resolved
@avpeery
Copy link
Contributor Author

avpeery commented Nov 2, 2021

This looks good. The only unclear thing I'm wondering about is whether any users depend on the deduplication logic for where a custom attribution and an attribution from a tile source may be partial duplicates — this would no longer work as custom attributions are passed as is.

@mourner I thought about this.. It seems to me it would be more on them to not have duplicate attributions in their custom attribution? But yes they would have to modify their current custom attribution if it is a duplicate of a source attribution.

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.

👍

@@ -186,6 +174,14 @@ class AttributionControl {
return true;
});

if (this.options.customAttribution) {
if (Array.isArray(this.options.customAttribution)) {
attributions = [...this.options.customAttribution.filter(s => typeof s === 'string'), ...attributions];
Copy link
Member

Choose a reason for hiding this comment

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

BTW, I'm not sure why we did this filter in the first place — it might no longer be relevant, possibly worth removing. Originally introduced in #7444

Copy link
Contributor Author

@avpeery avpeery Nov 4, 2021

Choose a reason for hiding this comment

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

without the check for string type, any data type can be passed through customAttribution, but is displayed ok in the attribution if we want to allow for that?

Copy link
Member

Choose a reason for hiding this comment

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

In theory it shouldn't matter, because we later do attributions.join(' | ') which automatically coerces each item into string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I removed the string filtering, just pushed the commit

@avpeery avpeery merged commit 1c7fc64 into main Nov 10, 2021
@avpeery avpeery deleted the avpeery/order-custom-attribution branch November 10, 2021 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

customAttribution does not respect order of array elements
2 participants