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

feat: use @opensystemslab/map web component #597

Merged
merged 11 commits into from
Aug 19, 2021

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Aug 11, 2021

We've been working on a web component map over here & it has feature-parity with planx requirements now!

Now using in:

  • FindProperty
  • DrawBoundary - including setting property.boundary.site & displaying area
  • Review

Other updates:

  • Importing via CDN script for now, but probably want to capture in package.json in future. will plan to replace CDN after fonts are packaged together with web component and we have a stable version
  • Removed all mapbox & turf dependencies! keeping @turf/helpers for Geometry type
  • Added new env var for OS Features API to editor .env & github actions secrets, should remove mapbox access token from github secrets after merge

@github-actions
Copy link

github-actions bot commented Aug 11, 2021

@netlify
Copy link

netlify bot commented Aug 11, 2021

✔️ Storybook

🔨 Explore the source changes: 1fa676d

🔍 Inspect the deploy log: https://app.netlify.com/sites/planx-storybook/deploys/611e40fa31514b0007144b86

😎 Browse the preview: https://deploy-preview-597--planx-storybook.netlify.app

// only a single polygon can be drawn, so get first feature in geojson "FeatureCollection"
setBoundary(geojson.features[0]);
});
}
Copy link
Member Author

@jessicamcinchak jessicamcinchak Aug 12, 2021

Choose a reason for hiding this comment

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

Curious if there are more react-y ways to approach this rather than event listeners?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough about Web Components to say, but if it was a react component I'd imagine we'd be passing in callbacks (e.g. onChange, onSelect) and those are really just glorified event listeners anyway 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

The idiomatic way to do it in react-land would be to create a reference with React.createRef then pass it down to <my-map> but I don't know if that would work or even make sense with web components (I need to learn more about web components).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok event listeners seem to be the way to go with web components

i just realized you'll need to remove those event listeners too on the cleanup stage of the effect hook

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, so much to learn about web components! I've been liking this guide so far for navigating between concepts: https://codelabs.developers.google.com/codelabs/lit-2-for-react-devs

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, updated to include cleanup function 🧹

Copy link
Contributor

Choose a reason for hiding this comment

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

lovely

@jessicamcinchak jessicamcinchak marked this pull request as ready for review August 12, 2021 19:01
@jessicamcinchak jessicamcinchak requested a review from a team August 12, 2021 19:01
@jessicamcinchak
Copy link
Member Author

⭐️Draw Boundary 2.0

@sarahscott
Copy link
Contributor

sarahscott commented Aug 14, 2021

This is coming along so nicely!! Do you want to update the hectares string bit before merging or should we review this as-is?

Copy link
Contributor

@gunar gunar left a comment

Choose a reason for hiding this comment

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

Absolutely love it!

// only a single polygon can be drawn, so get first feature in geojson "FeatureCollection"
setBoundary(geojson.features[0]);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The idiomatic way to do it in react-land would be to create a reference with React.createRef then pass it down to <my-map> but I don't know if that would work or even make sense with web components (I need to learn more about web components).

@jessicamcinchak jessicamcinchak merged commit 29b28bb into main Aug 19, 2021
@jessicamcinchak jessicamcinchak deleted the jess/osl-map-integration branch August 19, 2021 13:04
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.

3 participants