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

Custom Tile Layer for Map Component #1122

Merged
merged 8 commits into from
Jun 24, 2024
Merged

Conversation

willy1989cv
Copy link
Member

#1121 adds default tile layer and allows user to pass a tile object to map

#1121 adds default tile layer and allows user to pass a tile object to map
Copy link

vercel bot commented May 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

9 Ignored Deployments
Name Status Preview Comments Updated (UTC)
portaljs-alan-turing ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-ckan ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-ckan-ssg ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-fivethirtyeight ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-git-example ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-learn ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-openspending ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
portaljs-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm
site-portaljs ⬜️ Ignored (Inspect) Visit Preview Jun 24, 2024 8:43pm

Copy link

changeset-bot bot commented May 13, 2024

🦋 Changeset detected

Latest commit: 8e34967

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@portaljs/components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -14,6 +14,11 @@ import 'leaflet/dist/leaflet.css';
import * as L from 'leaflet';

export type MapProps = {
tile?: {
Copy link
Member

@demenech demenech May 13, 2024

Choose a reason for hiding this comment

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

IMO we should support both this flexible approach + the default providers from this lib https://github.com/leaflet-extras/leaflet-providers

This tile prop (which I think could be renamed to something like tileLayerSettings for more clarity) should have an interface like:


type UrlTileLayerSettings = { url: string }

// based on https://github.com/leaflet-extras/leaflet-providers
type MapboxTileLayerSettings = {     
  id: string,
  accessToken: string 
}   

type TileLayerSettings = UrlTileLayerSettings | MapboxTileLayerSettings // ...

...
tileLayerSettings: TileLayerSettings

Copy link
Member Author

Choose a reason for hiding this comment

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

added

tileLayerName: TileLayerPreset (MapBox, OpenStreetMap, Esri, custom, .... )
tileLayerOptions: L.TileLayerOptions 

cc @demenech

tile?: {
attribution?: string;
url: string;
data?: any;
Copy link
Member

Choose a reason for hiding this comment

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

Is this prop used anywhere?

Copy link
Member Author

@willy1989cv willy1989cv Jun 18, 2024

Choose a reason for hiding this comment

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

now this is all defined in TileLayerPresets object
cc @demenech

attribution='&copy; <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors'
url="https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png"
/>
<TileLayer url={tile.url} attribution={tile.attribution} {...tile.data} />
Copy link
Member

Choose a reason for hiding this comment

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

Based on which provider was chosen, we can pass different props to this component.

I think you can do an implementation based on this code https://github.com/leaflet-extras/leaflet-providers/blob/master/leaflet-providers.js

Copy link
Member Author

Choose a reason for hiding this comment

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

implemented this approach in latest commit
cc @demenech

@@ -43,6 +43,13 @@ type Story = StoryObj<MapProps>;
export const GeoJSONPolygons: Story = {
name: 'GeoJSON polygons map',
args: {
tile : {
Copy link
Member

Choose a reason for hiding this comment

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

We should document this new parameter on the story

Copy link
Member Author

Choose a reason for hiding this comment

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

will do it next @demenech

@anuveyatsu
Copy link
Member

Hey @willy1989cv let me know if you were able to address feedback from @demenech

@willy1989cv
Copy link
Member Author

Hey @willy1989cv let me know if you were able to address feedback from @demenech

@anuveyatsu @demenech At the moment I've implemented the more flexible approach, giving the user the possibility to configure the tile layer using component properties or .env variables.

https://hackmd.io/gqYVE-yhQu-iNnuu_m00Eg

@rufuspollock
Copy link
Member

@willy1989cv is this now good for a re-review?

@willy1989cv
Copy link
Member Author

@willy1989cv is this now good for a re-review?

Hi @rufuspollock. Working on the adjustments as suggested by @demenech.

@willy1989cv
Copy link
Member Author

@demenech please review latest changes.
here's a small doc on how it works: https://hackmd.io/@1kPvEKNGQAa77DV7pjEh5Q/Sk0geNvEA

Copy link
Member

@demenech demenech left a comment

Choose a reason for hiding this comment

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

Hey @willy1989cv

Very small change request, otherwise looks fine to me.

I'd like to suggest two other things:

  1. There should be a way to add markdown documents to Storybook stories, perhaps we could embed your documentation in the stories somehow?

  2. Just confirm this was tested on an external project after the project being built to ensure it works properly e.g. with the env vars

if (providers[providerName].url?.includes('{variant}') && !variantName)
variantName = Object.keys(providers[providerName].variants)[0];

console.log(variantName);
Copy link
Member

Choose a reason for hiding this comment

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

Remove log

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

@willy1989cv
Copy link
Member Author

willy1989cv commented Jun 20, 2024

@demenech working fine on my localhost @portaljs instance.

configured through properties:

# My Dataset

Built with PortalJS

## Map

<Map
    tileLayerName="MapBox"
    tileLayerOptions={{
        accessToken : "xxx"
    }}
/>

or via .env

NEXT_PUBLIC_MAP_TILE_LAYER_NAME=MapBox
NEXT_PUBLIC_MAP_TILE_LAYER_OPTION_accessToken=xxx

preview:
image

cc: @anuveyatsu

@demenech
Copy link
Member

hey @willy1989cv
that's great
let's add a changeset and merge

@demenech demenech merged commit 64103d6 into main Jun 24, 2024
10 checks passed
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
@demenech demenech deleted the feature/custom-tile-layer branch June 24, 2024 20:57
@willy1989cv willy1989cv mentioned this pull request Jul 1, 2024
5 tasks
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.

4 participants