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

Minimal 'CoreDataPlace' Widget Example #247

Closed
wants to merge 8 commits into from
Closed

Conversation

rsimon
Copy link
Collaborator

@rsimon rsimon commented Jan 27, 2024

In this PR

This PR adds a new component to the geospatial package: <CoreDataPlace>. This component retrieves a Place resource from the Core Data API, and renders it's GeoJSON geometry on a mapLibre map.

CoreDataPlace takes the following input props:

  • a mapLibre basemap style
  • the URI to the Place resource on the Core Data API
  • optionally: mapLibre style definitions for fillStyle, strokeStyle and pointStyle

Note: for now, this component is minimal, and supposed to serve as an example only!

Bildschirmfoto 2024-01-27 um 09 13 47

@rsimon rsimon requested a review from dleadbetter January 27, 2024 08:14
Copy link
Collaborator

@dleadbetter dleadbetter left a comment

Choose a reason for hiding this comment

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

This looks great! I might actually poach a couple of the components/styles for a backend Core Data feature I'm working on. A couple of questions/notes:

  • We're using FlowJS as a static type checker and ESLint for syntax. Could we make sure the code style is conforming to the rules we've setup?
  • Would it make sense to export the CoreDataPlaceLayer component? And add what is essentially the CoreDataPlace component (Peripleo, Map, etc) to the .stories file?

@@ -14,6 +14,8 @@
"@babel/preset-flow": "^7.22.5",
"@babel/preset-react": "^7.22.5",
"@faker-js/faker": "^8.0.2",
"@peripleo/maplibre": "^0.3.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also add these dependencies to the geospatial package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course - done!

@rsimon
Copy link
Collaborator Author

rsimon commented Jan 30, 2024

I made a few amendments:

  • Added @peripleo dependencies to geospatial.
  • Changed from interface to type.

I made a few tweaks to adapt the code style. I also ran yarn flow and that didn't report any errors. But not sure if I'm using it wrong...

Regarding the interface vs. type issue: that definitely was one of the issues. I'm one step further, but it's still not working. Storybook complains about @peripleo not exporting Feature and FeatureCollection for example. Both of which are interfaces. (In fact the log output lists all of the exports - and none of the interfaces are there.)

Is there anything we can change about the configuration to make Storybook recognize interfaces? Otherwise, I'll change all the interfaces defined in @peripleo to types.

@@ -22,6 +22,8 @@
},
"devDependencies": {
"@performant-software/webpack-config": "^1.0.0",
"@peripleo/maplibre": "^0.3.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be in dependencies instead of devDependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@@ -0,0 +1,80 @@
import React, { useEffect, useState } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you add the // @flow annotation to the top of this file, it will like the syntax a lot more. There's also a few ESLint errors on the formatting, but less concerned about those.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, but even with Flow enabled, I'm still getting red squiggles under every TypeScript statement, and Flow shows all the TS stuff as "uncovered area". I'll enquire with Camden.

@rsimon
Copy link
Collaborator Author

rsimon commented Jan 31, 2024

(Partial) success at last! Thanks for the hints - yes, those deps should have been under dependencies not devDependencies of course. Sorry for the mishap. (I shouldn't have copy-and-pasted those into the package.json after all...)

After that, the component showed up in storybook. A few things were still missing, which I added:

  • Import of essential CSS styles for Peripleo + minimal CSS config needed for the component
  • Changed the demo map style to one that works without a MapTiler API key
Bildschirmfoto 2024-01-31 um 07 53 24

I also added the // @flow directive. This did seem to change at least something. (The Flow status indicator in VS code now shows that the file is begin covered.) But otherwise had no effect on syntax highlighting. Still getting errors shown about TS not being allowed in .js files. I'll enquire with Camden.

@dleadbetter
Copy link
Collaborator

Closing in favor of #252.

@dleadbetter dleadbetter closed this Mar 4, 2024
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