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

Default attribution #3618

Merged
merged 20 commits into from
Jan 28, 2024
Merged

Default attribution #3618

merged 20 commits into from
Jan 28, 2024

Conversation

HarelM
Copy link
Member

@HarelM HarelM commented Jan 25, 2024

Launch Checklist

Update icon, add attribution by default.
I find the configuration a bit too verbose both for the logo and attribution, maybe we could reduce some of the configuration by using a single parameter for each, IDK...

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Document any changes to public APIs.
  • Add an entry to CHANGELOG.md under the ## main section.

New logo, not on by default:
image

Default attribution now on:
image

@HarelM
Copy link
Member Author

HarelM commented Jan 25, 2024

CC: @wipfli you can try it out.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8ce93cb) 86.91% compared to head (ff1cc97) 87.19%.

Files Patch % Lines
src/ui/map.ts 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3618      +/-   ##
==========================================
+ Coverage   86.91%   87.19%   +0.27%     
==========================================
  Files         240      240              
  Lines       32211    32214       +3     
  Branches     2092     2093       +1     
==========================================
+ Hits        27997    28088      +91     
+ Misses       3272     3202      -70     
+ Partials      942      924      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wipfli
Copy link
Member

wipfli commented Jan 26, 2024

Thanks for writing this pull request @HarelM. I wanted to try it out locally with the examples, but looks like they use the published version of GL JS. Is that right?

@wipfli
Copy link
Member

wipfli commented Jan 26, 2024

image

@HarelM
Copy link
Member Author

HarelM commented Jan 26, 2024

Use npm run start and surf to the relevant example.
Yes, docs are using the published version.

@wipfli
Copy link
Member

wipfli commented Jan 26, 2024

So basically to remove the "MapLibre" in the attribution users have to do this:

const map = new maplibregl.Map({
    // ...
    attributionControl: false,
});

map.addControl(new maplibregl.AttributionControl({
    customAttribution: ''
}));

is that correct?

@wipfli
Copy link
Member

wipfli commented Jan 27, 2024

I think we should use the new logo from #3631 which is gray scale rather than this version here...

@HarelM
Copy link
Member Author

HarelM commented Jan 27, 2024

Feel free to push updates to this branch. I have no hard feelings either way.

@HarelM
Copy link
Member Author

HarelM commented Jan 27, 2024

Make sure you remove all the extra annotations from the svg to reduce svg size in production.

src/ui/map.ts Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

I think a grayscale logo has a better chance of being used by people.

@HarelM
Copy link
Member Author

HarelM commented Jan 28, 2024

I don't think people will voluntarily add the logo to their app, so I think the logo is a better approach in terms of "marketing", but I don't have strong feelings either way.
If we don't add it by default we might as well remove the control, that's what I think at least.
If you don't know what you are doing and all you need is to show a map, it is likely that you'll keep the logo/attributnion.
That's my two cents.
In any case, changing this is very easy.

@louwers
Copy link
Collaborator

louwers commented Jan 28, 2024

@HarelM Well Komoot proudly gives MapLibre credit for example.

And not everybody is building an app with MapLibre. When someone includes a map in a blog post for example, I can imagine people don't mind showing the logo.

@HarelM
Copy link
Member Author

HarelM commented Jan 28, 2024

So what about the grayscle icon (the other PR which should target this branch)? Should we use it or not?

@louwers
Copy link
Collaborator

louwers commented Jan 28, 2024

Yes let's use that one!

@wipfli
Copy link
Member

wipfli commented Jan 28, 2024

Looks good to me. Thanks for working on this @HarelM!

@HarelM HarelM enabled auto-merge (squash) January 28, 2024 18:33
@HarelM HarelM merged commit 3fdea0d into main Jan 28, 2024
15 checks passed
@HarelM HarelM deleted the default-attribution branch January 28, 2024 18:37
@breunigs
Copy link

Was it intentional for the attribution to be always collapsed by default? I can't quite tell from the PR description. If so, maybe the sentence that recommends to keep it expanded (https://github.com/maplibre/maplibre-gl-js/pull/3618/files#diff-7489f127e6e44739b4d10d3d5102ca30e47ff444ebba85ea9921e558e9468676L14) should be removed, as well guidance on how to get "automatic" mode back (passing anything but a boolean works, but it's not obvious when just reading the docs)

@HarelM
Copy link
Member Author

HarelM commented Mar 25, 2024

As can be seen here:
https://maplibre.org/maplibre-gl-js/docs/examples/simple-map/
The default is not collapsed on desktop without moving the map, once the map pans the attribution will collapse or when surfing from a mobile device (small screen).
Feel free to open a PR to update the docs if you think they can be improved.

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.

None yet

5 participants