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 the map module to the admin styles #8043

Merged
merged 2 commits into from Jun 7, 2021

Conversation

ahukkanen
Copy link
Contributor

@ahukkanen ahukkanen commented May 24, 2021

🎩 What? Why?

When static maps are displayed through Leaflet (which is the case when static maps are not available), the map squares in the admin panel are invisible.

This adds the map CSS to the admin panel to fix the issue.

📌 Related Issues

Testing

  • Enable maps
  • Disable static maps service as described in the maps configuration documentation
  • Go to the admin panel to see some record with a map position
  • See that the map is not displayed

📋 Checklist

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Static map replacement in the admin panel

@leio10
Copy link
Contributor

leio10 commented Jun 7, 2021

@ahukkanen I don't understand exactly how is this fixing the problem. I understand that the added mixin was missing in the admin styles, as it is only defined in the front styles. But I don't get the whole picture. Are you fixing the static_map_link or the dynamic_map_for helper? Maybe the description of the PR is wrong, with some static mistaken with dynamic? I didn't find the place where those helpers are used in the admin zone.

Please, give me a clue to follow the fix and I'll merge this ASAP. 🙏
Thanks! ❤️

@ahukkanen
Copy link
Contributor Author

@leio10 All map providers don't provide static map API which provides static map images.

If you now go to admin panel looking at some record with static map displayed, it will show properly with HERE.

If you follow the testing instructions that I wrote in the issue description and disable the static maps utility, you will see that the static map images disappear from admin panel.

The only relevant part what we need to fix this is this commit:
7d0cfa2

This commit is only necessary because the core map module needs these and won't compile if these are not available:
b86b585

@leio10
Copy link
Contributor

leio10 commented Jun 7, 2021

@ahukkanen Ah, ok, I got the fix part, but I still don't understand this paragraph:

When static maps are displayed through Leaflet (which is the case when static maps are not available), the map squares in the admin panel are invisible.

I don't understand how "static maps" are displayed if they are not available. I can think three options here:

  • "Static maps" has two meanings: maps that are not interactive in the first case and a service of image-only maps in the second.
  • There is a typo, and the first "static maps" should be "dynamic maps"
  • There is another explanation that I don't get 😅

@ahukkanen
Copy link
Contributor Author

  • "Static maps" has two meanings: maps that are not interactive in the first case and a service of image-only maps in the second.

Yes, I think this one. So, if "static maps" (in the actual meaning, static map images) are not available, Decidim will fall back to displaying a non-interactive static map replacement with Leaflet. This replacement is displayed with Leaflet but it is non-interactive, very similar to the normal "static map". I call them both "static maps".

So, if the "static" maps are displayed in admin panel through Leaflet, they won't be visible without this fix.

@leio10 leio10 merged commit c72edbe into decidim:develop Jun 7, 2021
@ahukkanen ahukkanen deleted the fix/static-leaflet-maps-in-admin branch June 7, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review module: admin type: fix PRs that implement a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants