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

✨attributionButtonPosition for web #304

Merged
merged 10 commits into from
Oct 4, 2023
Merged

✨attributionButtonPosition for web #304

merged 10 commits into from
Oct 4, 2023

Conversation

ouvreboite
Copy link
Contributor

@ouvreboite ouvreboite commented Sep 27, 2023

Fix for: #283

  • implement attributionButtonPosition for the web target
    • in maplibre_gl_web/lib/src/mapbox_web_gl_platform.dart, we default to attributionControl: false in the _map constructor, as the attributionControl is set later when setAttributionButtonAlignment is called.
    • ℹ️ It's similar to the example given in https://maplibre.org/maplibre-gl-js/docs/examples/attribution-position/ that also sets attributionControl to false in order to set manually the position.
  • interop scaffolding done
  • new "attribution" example page
    • it uses a osm_style.json exposed as an asset, as the web target does not support hardcoded styles
    • the no_location example is updated to also use osm_style.json, to make it work correctly in web

Attribution example page
attribution_example

@m0nac0
Copy link
Collaborator

m0nac0 commented Oct 1, 2023

For me on Firefox and Chromium browsers on Windows, the attribution looks wrong, it's just red text and has no white background and no info icon. Do you have any idea of what might be going wrong?

Edit: This actually also occurs with the example app from the main branch (but can only be seen when I change the no_location_permission page such that the style is not inlined, because otherwise the style is not loaded at all)

JulianBissekkou pushed a commit that referenced this pull request Oct 2, 2023
…307)

This occurred for example in the example app on the animated camera
control page or the attribution page from #304
@m0nac0
Copy link
Collaborator

m0nac0 commented Oct 2, 2023

Two more points:

  • the path to the style asset seems to be wrong and doesn't work on Android/iOS. I think it should be assets/osm_style.json
  • passing null for attributionButtonPosition does not hide the attributionButton on Android/iOS in the existing implementations. So for now I think web should also use the default position when null is passed. To show no attribution, there was a previous PR Remove attribution button #174 using a new parameter for that. We could also discuss your approach, but I think that should be then implemented on all 3 platforms and it would be a breaking change.

I also noticed while testing this PR that on Android, attribution for the sources is currently missing from the attribution dialog. I opened #308 for this, but it is unrelated to your work

example/lib/attribution.dart Outdated Show resolved Hide resolved
example/lib/attribution.dart Outdated Show resolved Hide resolved
example/lib/attribution.dart Outdated Show resolved Hide resolved
@ouvreboite
Copy link
Contributor Author

@m0nac0 any other things you would like me to address ?

Copy link
Collaborator

@m0nac0 m0nac0 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution and your patience! Looks very fine to me.

@m0nac0 m0nac0 merged commit cd5950d into maplibre:main Oct 4, 2023
6 checks passed
@m0nac0
Copy link
Collaborator

m0nac0 commented Oct 4, 2023

For me on Firefox and Chromium browsers on Windows, the attribution looks wrong, it's just red text and has no white background and no info icon. Do you have any idea of what might be going wrong?

Edit: This actually also occurs with the example app from the main branch (but can only be seen when I change the no_location_permission page such that the style is not inlined, because otherwise the style is not loaded at all)

By the way: I solved this by upgrading flutter.

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.

2 participants