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

Add Multipoint support with useGeometry composable #190

Closed
wants to merge 2 commits into from

Conversation

wazolab
Copy link
Member

@wazolab wazolab commented Mar 8, 2024

See a basic exemple here: https://gist.github.com/wazolab/1338938826b9c317184bec64c5af6980

@frodrigo: For the moment I'm not sure the integration is perfect in Vido. I need to go deeper into map rendering lifecycles to be sure the feature flattening is always effective.

Can you tell me if the function suits our needs for the moment ?

@wazolab wazolab added the feature label Mar 8, 2024
@wazolab wazolab self-assigned this Mar 8, 2024
@wazolab wazolab linked an issue Mar 8, 2024 that may be closed by this pull request
@frodrigo
Copy link
Member

frodrigo commented Mar 8, 2024

See also this commit

0863092

Include a hack to get the MultiPoint into the app.

  • Not working in category unselect/switch, but initial load
  • But cluster still missing icon on unziped cluster

image

@wazolab wazolab force-pushed the 189-add-multipoint-support branch from d3e4ae3 to 0af8505 Compare March 8, 2024 16:32
@@ -191,6 +191,9 @@ export const menuStore = defineStore('menu', {
)
).filter(e => e) as ApiPois[]

const { flattenMultipoint } = useGeometry()
posts = posts.map(p => ({ ...p, features: flattenMultipoint(p.features) as ApiPoi[] }))
Copy link
Member

Choose a reason for hiding this comment

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

No need for composable here, as std import is fine. Or even move the code here as it the only use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course we can make flattenMultipoint as default export of useGeometry composable.
I just dit it this way for separation of concerns :)
I think copying the function straight to menu.ts will just increase the complexity of understanding and maintainability.
I could've think about a store getter, but I think the correct usage of getters is when you want a transformated format of state value.
In our case we want the state value to be always transformated, so I doesn't seems to be a correct option.

@wazolab wazolab force-pushed the 189-add-multipoint-support branch 2 times, most recently from 18e857e to 051774f Compare March 12, 2024 19:46
@frodrigo
Copy link
Member

I confirm the issue on marker come from the id
17150793#diff-3211ebb376981abe96fcf7a34cfa1ce90292212544996ab6a265ae470b107022R189

The issue about color of cluster come from usage of unknown color on the POI. The color should be one the menu. We only have stats for colors from menu.

@wazolab
Copy link
Member Author

wazolab commented Mar 13, 2024

Ok so we have two separated issues if I understand well ?

  • Wrong Id
  • Missing color in Menu

About the ID could you explain why we are doing id: feature.properties.metadata.id + feature.properties.metadata.id * 100000 + index ?

@frodrigo
Copy link
Member

About the ID could you explain why we are doing id: feature.properties.metadata.id + feature.properties.metadata.id * 100000 + index ?

Generate a diff id for each new Point from MultiPoint due to this https://github.com/teritorio/vido/blob/develop/lib/markerLayerFactory.ts#L212

* Missing color in Menu

That only a settings issue.

@wazolab wazolab force-pushed the 189-add-multipoint-support branch 2 times, most recently from dea2f8d to 9809bb2 Compare March 13, 2024 11:02
@frodrigo
Copy link
Member

  • When MultiPoint only in the view port, the app ask to dezoom. A bbox computation MultiPoint should missing somewhere.

@wazolab wazolab force-pushed the 189-add-multipoint-support branch from 9809bb2 to 9602456 Compare March 18, 2024 13:56
@wazolab
Copy link
Member Author

wazolab commented Mar 18, 2024

Should I close this PR as #203 fixes it ?

@frodrigo frodrigo closed this Mar 18, 2024
@frodrigo
Copy link
Member

Closed in favour of #203

@wazolab wazolab deleted the 189-add-multipoint-support branch April 9, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Multipoint support
2 participants