search

CartoDB / toolkit

JS library to interact with CARTO APIs in a simple way
https://toolkit-wheat.now.sh
BSD 3-Clause "New" or "Revised" License
9 stars 3 forks source link

Adapting popups & interactivity to the new event mechanism #123

Closed manmorjim closed 4 years ago

vercel[bot] commented 4 years ago

This pull request is being automatically deployed with Vercel (learn more). To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carto-frontend/toolkit/jrfg2lgeq ✅ Preview: https://toolkit-git-mmorillo-ch76516popup-interactivity-a-92cce2.carto-frontend.now.sh

manmorjim commented 4 years ago

I've just added small changes in the Layer class so only this class shoud be reviewed.

I also added the mitt dependency to the viz package.json because it needs it. I'm not sure about this, maybe a better option would be just exporting the mitt.Handler type?

jesusbotella commented 4 years ago

I've just taken a look at this, and the synchronization that I built between the Dataviews and the Layer would be OK. So no more changes needed.

My point was to use that event management that I built to synchronize Deck.gl events and our events for popups and avoid having two different event management approaches (the more general one and the one for the interactivity). That way we can remove some repeated code and make it more manageable, but we can leave it for a refactor later as it would require a big effort.

manmorjim commented 4 years ago

Changed Popup in order to allow register severals onViewStateChange events: https://github.com/CartoDB/toolkit/pull/123/commits/3fe53bc2e03ab030a45f2b428b32e3a270ffb231

manmorjim commented 4 years ago

Refactor done!

VictorVelarde commented 4 years ago

@manmorjim looks good to me. Let's ping @jesusbotella to check if we can now go and merge this into dataviews...

manmorjim commented 4 years ago

The only thing that I see is that you can call off method but it doesn't mean that you are unregistering the handler because it can be another handler that doesn't match the current ones. So not sure that we should always decrement the variable.

Yeah, good catch :+1: We should think about a mechanism to get all the handlers attached to an instance