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

Style Switcher for Examples #4813

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ianthetechie
Copy link
Contributor

I'm opening this PR as a draft first as it is still incomplete, but want to get feedback on the overall approach before proceeding.

Summary

This PR introduces a wider set of basemaps that will cover most examples.

The MapLibre demo tiles work for some examples, but are not great at showcasing MapLibre's full capabilities. As such, MapLibre GL JS examples currently use tiles from several providers (the vast majority being MapTiler, which whitelists the example traffic). Since MapLibre is a vendor-neutral project and most examples are not specific to any particular provider, it makes sense to be able to switch examples between several options. This will result in greater awareness for MapLibre and better examples, since tile providers are incentivized to improve and update the examples, and link directly to them from their own sites.

Where possible (some examples may justifiably be limited to specific providers), I propose that the Americana stylesheet should be the default where we need a more "full" basemap. Other providers will be listed in alphabetical order (full disclosure: I'm affiliated with Stadia Maps, and the proposed sorting puts us at the bottom of the current list).

My reading of the Americana Tile Usage Policy is that MapLibre usage falls under the acceptable usage policy. Though I think we probably need to reach out as the maplibre.org domain, while (I think) hosted on GitHub pages, seems to use CloudFlare in front.

Technical discussion

Deciding when to display a style switcher

Examples that meet some criteria (at doc generation time) will have a style switcher. Currently, the criteria is "does the example use the OSM Americana style URL, since I'm proposing that as the default. I have updated a handful of examples to illustrate this. The rest of the examples will be addressed one-by-one after we agree on the approach.

An alternative would be to include some metadata at the top of the example (more on that later). I have opened this draft without that, as I think it unnecessarily complicates the majority of examples.

Examples that do not pass the "should we have a switcher" check are displayed as they were before. This allows for examples that are provider-specific or only make sense with a single style.

Style switcher implementation

The style switcher is currently a select box added via a map control. We could devise something that looks a bit nicer perhaps, but I opted for this as it's simple and looks reasonably good. The control is injected into the documentation (iframe component) if the generator determines that the example should have a style switcher. This JavaScript is obviously not included in the copyable sample code; it's localized to the iframe.

Changing the selection map style will update the URL with a query string parameter with a style slug. This parameter will be consulted on page load so that copying a URL will let the recipient see the same style.

Changing the map style will also update the example code! The way this currently works is admittedly a bit hackish, since we have to work with a loaded DOM. I believe the method is sufficiently robust, but am open to alternatives. The only MkDocs-native methods I could think of (generating a bunch of tabs with code fences that are 99% duplicate code) would multiply the size of the examples, and would be difficult if not impossible to sync with the map view.

Customizing the set of displayed styles

The set of available map styles can be overridden per example.

The examples are currently plain HTML files, so in the interest of keeping it that way and localizing overrides, I've opted for parsing a leading HTML comment as a JSON configuration (sort of like YAML frontmatter, but we can't have nice things like that in HTML). The generator removes the parsed comment from the example.

<!--
{
  "availableMapStyles": {
    "mapTilerHybrid": {"name": "MapTiler Hybrid", "styleUrl": "https://api.maptiler.com/maps/hybrid/style.json?key=get_your_own_OpIi9ZULNHzrESv6T2vL"},
    "alidadeSatellite": {"name": "Stadia Maps Alidade Satellite", "styleUrl": "https://tiles.stadiamaps.com/styles/alidade_satellite.json"}
  }
}
-->

This is admittedly hackish, but I believe it's robust enough. We already use similar approaches when parsing JS plugins in the doc from the awesom-maplibre repo. The other idea I've considered so far is putting the overrides directly in generate-docs.ts. This is worse in my opinion, since you would need to look at the generator script to understand how an example behaves. That felt wrong.

Known issues

  • Going back after changing styles appears to work. However, more complex history manipulation sequences like going forward again, going back multiple times, etc. seem to have no effect on the map style switcher.
  • I haven't updated any example thumbnail images

Screenshots

Before

image

After

style-switcher-demo

Launch Checklist

  • 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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.


try {
config = JSON.parse(configBody);
const htmlContent = rawHtml.replace(configPattern, '').trim();

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High

This string may still contain
<!--
, which may cause an HTML element injection vulnerability.
@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.76%. Comparing base (4d8a0f8) to head (614f1ad).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4813      +/-   ##
==========================================
- Coverage   87.87%   87.76%   -0.11%     
==========================================
  Files         265      265              
  Lines       37577    37590      +13     
  Branches     2349     2359      +10     
==========================================
- Hits        33020    32991      -29     
- Misses       3518     3561      +43     
+ Partials     1039     1038       -1     

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

@wipfli
Copy link
Contributor

wipfli commented Oct 9, 2024

Thanks for opening this pull request and showcasing how different tile providers could be handled on a technical level, @ianthetechie.

The MapLibre Governing Board is in the middle of drafting a policy for the examples and I suggest to wait for the new policy before changing the current state of the docs website.

Thanks for your patience!

@HarelM
Copy link
Collaborator

HarelM commented Oct 10, 2024

Besides what @wipfli mentioned, here are my 2 cents:

  1. We should be using a standard layer switcher like the following: https://github.com/korywka/mapbox-controls/tree/master/packages/styles, I'm not sure if this is the best choice or we should add it to maplibre much like other controls we have, leaflet has a nice icon to it and it is included to allow easy style switching, I think it worth the 100 or so lines of code internally, or a dedicated plugin.
  2. One of the things I tired the most with the examples is that you can use them for debug locally when developing, and you can also run them locally to see if there are issues. I would like to keep it that way as much as possible, i.e. no complicated manipulation of the html in order to inject stuff (we already do some manipulation of the js script file, but that's mostly it). if we want a json file the is loaded to indicate which styles are available for use instead of harcoding it in every example, that's fine, and we have some external json resources for the examples, such as earthquake data in assets which we can use similarly. Bottom line, what I'm trying to emphasize here is what you see is what you get approach for the example, at the cost of adding 1-3 lines to each example to load the layer switcher.

Hope that makes sense. I don't have hard feelings either way. But the examples and docs were extremely complicated when I rewrote them, and I would like to avoid getting back to a complicated docs build scripting as much as possible...

@birkskyum
Copy link
Member

birkskyum commented Oct 10, 2024

@ianthetechie thanks for the PR. It'd like to see the scope of this work be reduced to the style switcher functionality itself, focusing on the feedback @HarelM mention, and leaving out any changes to which styles are used in the examples. Introducing the capability of listing multiple styles is by itself not controversial, and it brings a degree of freedom to the policy negotiations that @wipfli mentioned (which styles to list; which to have default).

I don't know if it's the case already, but it's necessary we can set a default / order, and not only have it alphabetic.

@ianthetechie
Copy link
Contributor Author

We should be using a standard layer switcher...

I thought the same thing initially. I liked the look and feel of mapbox-controls, but it could break with Mapbox changes in the future. Do we want to bring in a dependency that might break later? If it's not an issue, I can easily rework to that.

One of the things I tired the most with the examples is that you can use them for debug locally when developing, and you can also run them locally to see if there are issues. I would like to keep it that way as much as possible, i.e. no complicated manipulation of the html in order to inject stuff (we already do some manipulation of the js script file, but that's mostly it).

Yeah, I think I understand what you're getting at here. My approach so far preserves that, I think? As in, you can just open up any HTML file (with a localhost web server I presume for commercial styles as I think MapTiler also allows this?). No changes needed, the example "just works."

The style switcher is added on separately for now because this isn't important to the example. If we added the style switcher directly in the example code, we would have to process that out, which gets a lot trickier. Adding the "metadata" about which styles are supported via a comment preserves the "just works locally" nature of the examples, and makes them easy to copy+paste.

I'm not sure if you were also (in?)directly addressing the fact that the style switcher manipulates the example HTML as well, but I think this is a useful feature because it means users can copy any example and reproduce it (sans API key perhaps) locally, and the result will look just like the example.

Let me know if I've missed/misunderstood something in this discussion @HarelM :)

But the examples and docs were extremely complicated when I rewrote them, and I would like to avoid getting back to a complicated docs build scripting as much as possible

Understood and agreed 😅

@ianthetechie
Copy link
Contributor Author

Thanks for the comments @birkskyum!

It'd like to see the scope of this work be reduced to the style switcher functionality itself... leaving out any changes to which styles are used in the examples.

Fair enough! Apologies if this was perceived as a recommendation on specific styles to include. The easiest way to implement a style picker selectively was to look for a known (default) URL. I think it's important to point out that some examples today are highly specific to one tile provider (ex: some that make modifications that are specific to MapTiler), or that make assumptions about the schema. I'm perfectly fine with both, so my suggestion was to set a default style because it's easy to just look for that and assume that styles using a different default are not going to use the switcher.

I don't know if it's the case already, but it's necessary we can set a default / order, and not only have it alphabetic.

The current implementation is that the default is sorta hard-coded, and the rest appear in the specified order. I chose alphabetical ordering because it was neutral.

I think the two big outstanding questions on how to best implement this are as follows:

  1. How do we set a default style without hard-coding one into every example? This seems to be at odds with the desire to keep examples locally usable. I avoided any deeper changes because I wanted to avoid complicating things, but the cleanest way I know of would be to make the examples templates with a placeholder for the default style. However, this breaks a critical property that @HarelM highlighted of being able to easily test anything locally. Any ideas? Or maybe I misunderstood?
  2. How should we allow overrides per-example? Is the HTML comment parsing solution acceptable? Or are we wanting to do something different? Despite being a bit hackish, it does have an important property of making the example valid as standalone HTML. If we had to preprocess it, then that property is gone. We technically preserve it if we load an external JSON file (checked into the repo), but we'd have to do even more processing of the HTML before it gets to the iframe content version to remove the style switcher specifics. Removing the comment after parsing by contrast is quite straightforward and the original HTML remains valid.

@HarelM
Copy link
Collaborator

HarelM commented Oct 14, 2024

Let's wait for the board decision on this before we progress any further.

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.

5 participants