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

[WIP] Custom JS injection #191

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

[WIP] Custom JS injection #191

wants to merge 20 commits into from

Conversation

aparlato
Copy link
Collaborator

@aparlato aparlato commented Apr 30, 2024

Description

Downstream client is looking for a way to inject custom JS actions to act on the map through Maperture. This PR experiments with one way to handle this. This would be an advanced feature that we would likely discourage for typical use.

Functions are passed separately in the config as we handle the map objects (presets, etc) in a way that needs to go between strings and objects which removes functions.

The function passed should be a factory function that takes a map argument and spits out a function that acts on that map to be run on clicking the option. These changes do not persist if the map style is changed or closed. They do not persist across styles.

[
  {
    type: 'dropdown',
    label: 'Backgrounds',
    mapIds: ['my-cool-map'],
    options: [
      {
        label: 'Blue',
        script: map => {
          return () => {
            map.setPaintProperty('background', 'background-color', 'blue');
          };
        },
      },
      {
        label: 'Red',
        script: map => {
          return () => {
            map.setPaintProperty('background', 'background-color', 'red');
          };
        },
      },
    ],
  }
]

TODO

  • Document syntax
  • Add other types than dropdown (maybe button)
  • Add non-mapbox/maplibre maps to the map object store
  • Make sure map id changes are fully propagated through and don't cause bugs

Closes #

QA steps

  • ...

Author checklist

Create the PR

  • Fill out PR template
  • Make sure you've added a CHANGELOG entry under "Unreleased"
  • Request a review
  • Make any requested changes and get approval

After approval

  • Merge any changes from main into your branch, resolve any conflicts
  • Squash & merge changes into main
  • Delete branch

@aparlato
Copy link
Collaborator Author

aparlato commented May 6, 2024

@ebrelsford I have a few todo's in here, but am already using this downstream off this branch and it meets our needs. If this seems like an acceptable advanced feature, I can go ahead and finish those up before putting up for official review. Do you see any issues with this as a feature or with the implementation?

@ebrelsford
Copy link
Contributor

@aparlato This approach generally looks good to me!

Maybe this is for future consideration, but I do wonder if there's a case to be made for completely transforming a style? For example if you want to change all instances of a given field name or color in paint properties or something simpler like changing glyphs.

@aparlato
Copy link
Collaborator Author

aparlato commented May 6, 2024

Maybe this is for future consideration, but I do wonder if there's a case to be made for completely transforming a style? For example if you want to change all instances of a given field name or color in paint properties or something simpler like changing glyphs.

If I'm understanding the ask/suggestion here correctly, I'd rather put the onus of that on the user to write their custom script. Partially because it's more flexible and partially because it's an advanced feature I wouldn't want to encourage by making too user friendly. I'll send you an implementation of this in Slack that does this as I don't want to post publicly on this PR.

@ebrelsford
Copy link
Contributor

Yeah maybe at some point we just document that as a possibility for when it comes up.

@aparlato
Copy link
Collaborator Author

Looks like the customJs property accidentally became required in the code: See CustomJsUi.svelte:17

Can be worked around by using an empty array.

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