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 option to collapse attributions by default in toggle mode #4526

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Aug 8, 2024

It's been requested ( plotly/plotly.js#7076 (comment) ) to have a setting that'll allow initially collapsing the attribution text when it's in toggle-mode (compact or screen smaller than 640px).

Currently, attribution will collapse on drag, but for plotting libs like plotly.js show below, there's typically many small figures (attribution is repeated and takes up relatively more space), with the same viewport (dragging puts them out of sync). This PR adds an opt-in to the AttributionControl called collapsedByDefault to to collapse attributions by default.

Screenshot 2024-08-08 at 18 35 20
  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

@birkskyum birkskyum changed the title Add option to initially hide attributions on small screens Add option to initially hide attributions in small figures Aug 8, 2024
@birkskyum birkskyum requested a review from HarelM August 8, 2024 16:48
@birkskyum birkskyum changed the title Add option to initially hide attributions in small figures Add option to collapse attributions by default in small figures Aug 8, 2024
@birkskyum birkskyum changed the title Add option to collapse attributions by default in small figures Add option to collapse attributions by default Aug 8, 2024
@birkskyum birkskyum changed the title Add option to collapse attributions by default Add option to collapse attributions by default in compact/small figure mode Aug 8, 2024
@birkskyum birkskyum changed the title Add option to collapse attributions by default in compact/small figure mode Add option to collapse attributions by default in small figures Aug 8, 2024
@birkskyum birkskyum changed the title Add option to collapse attributions by default in small figures Add option to collapse attributions by default in toggle mode Aug 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (ba721b4) to head (20cf5c7).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4526      +/-   ##
==========================================
+ Coverage   87.79%   87.86%   +0.07%     
==========================================
  Files         246      246              
  Lines       33450    33462      +12     
  Branches     2219     2222       +3     
==========================================
+ Hits        29367    29402      +35     
+ Misses       3081     3053      -28     
- Partials     1002     1007       +5     

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

@HarelM
Copy link
Collaborator

HarelM commented Aug 8, 2024

If you touch the map the attribution will collapse, is this really a must?
I'm guessing (not sure though) this can also be done simply with CSS so no API changes are required...?
If there's no other alternative, please add a test.

@acalcutt
Copy link
Contributor

acalcutt commented Aug 8, 2024

The point of starting the attributions expanded in #795 was to comply with OpenSteetMap Attribution Guidelines. Based on the screenshots above it looks like OSM is being used there, so adding this option goes against the spirit of the license (in my opinion).

It might not be bad to have an option, but in using it it should be clear that the user should take other steps to make attribution visible (like showing it in another place or method) to comply with the OSM license (if OSM is used)

For example, if those multiple images are non-interactive, maybe attributions could disable it in the maps, but add the attribution to a single spot on the page, like they mention for static images

Static images
Static images must be generally attributed the same way as interactive maps. However, if multiple static images appear on the same document, one instance of attribution is sufficient

@birkskyum
Copy link
Member Author

birkskyum commented Aug 9, 2024

@acalcutt , this is technically Interactive Maps, unless screenshotted and used on print ofc. (!) , which means the only hard requirement of the linked license I can see is:

If the attribution has been collapsed, the user must still be able to find the licence information if they look for it, for example from an ‘(i)’ button in the corner of the map or an ‘About’ option in a menu.

I am not a lawyer, and this ticket wan't specifically about osm which just turned out to be an interesting example, but I don't think it technically breaches the license of OSM to have it hidden by default given the above extract, if a user of maplibre want to do that. This other extract, from the osmf-talk in 2021 tells a different story, so maybe the license will change at some point:

We set an expectation that attribution is visible for at least some time without user interaction to fall within the safe harbour outlined by the Attribution Guidelines

I don't have much opinion in the matter myself, and I can sympathize with the "spirit of the license" you mention which is also why this is an opt-in. At the end of the day I don't really want to tell anyone how to comply with various licenses, but rather just provide the flexibility necessary to do so and have good defaults. We can in the docs of this flag write that users should be wary of complying with the attribution license of the style / data-provider in question, in case there are styles out there with a legal requirement to be present initially or even at all times.

@HarelM , I did try a bit with the css, but the problem I hit was that the -show class is added to the dom element initially, and I can't remove it from the css. This would do so that the -show isn't added initially if this flag is set.

@archmoj , let me know if this is still a blocking issue for the next RC of plotly.js.

@acalcutt
Copy link
Contributor

acalcutt commented Aug 9, 2024

I think you missed the bit right above it

You may use a mechanism to fade/collapse the attribution under certain conditions:

immediately with a dismiss interaction, for example clicking an ‘x’ in the corner of a dialog
automatically on map interaction such as panning, clicking, or zooming
automatically after five seconds. This also applies to splash screens or pop-ups.

Maplibre I don't think is obligated to comply with the osm license, but it should be able to. before it wasn't possible to to comply with any of those recommended options, but right now we comply with the second one. With your option at least it would allow the user to change the default if they did not need to comply with those rules (like if they used another source with a different license).

We have had other conversation about this recently in the maplibre-native side in maplibre/maplibre-native#2600

@birkskyum
Copy link
Member Author

birkskyum commented Aug 9, 2024

@acalcutt , you're right, I read that second hard requirement wrong, so it's only if it's a non-osm data/style that this setting comply with the license. I think we also comply to some extend with the first, since the toggle can be clicked to dismiss the attribution, without making a map interaction (depending on definition) such as panning, clicking, or zooming.

@acalcutt
Copy link
Contributor

acalcutt commented Aug 9, 2024

ya, we can also comply with the first option right now also, as "toggle can be clicked to dismiss the attribution", but it must be shown first (which is what my change in #795 did)

the automatic close on map panning was more so the user did not have to click the button themselves to hide it (when in compact mode)

@acalcutt
Copy link
Contributor

acalcutt commented Aug 9, 2024

I think right now if you really needed this, you might be able to set your own blank attribution on your sources. If there are no attributions I had made it so the compact toggle button doesn't even show.... idk i would have to test that. you would still need to attribute elsewhere though.

Edit:
Someone could also explore the third option for closing, automatically after five seconds.

@birkskyum
Copy link
Member Author

apple maps only has a tiny "Legal" link in the lower right... @acalcutt is that within the rules, these way you interpret then?

imageimage

@acalcutt
Copy link
Contributor

acalcutt commented Sep 24, 2024

Personally I don't think the apple implementation does meet the guidelines, but they have better lawyers than me :-) . I will also note the change to require showing it isn't very old (it was changed in the last few years ago), so it likely was enough before that change.

I'd say what apple has done is similar to maplibre-native implementation talked about in maplibre/maplibre-native#2600

@acalcutt
Copy link
Contributor

acalcutt commented Sep 24, 2024

I don't think adding and option would be a bad thing though. It would be up to the person deploying the map, At least we have safe defaults.

@HarelM
Copy link
Collaborator

HarelM commented Sep 25, 2024

I'm fine either way, I don't have strong feelings.
Please add the relevant unit tests if you would like to push this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants