Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

Exchanges: map rendering #40

Merged
merged 11 commits into from
Nov 18, 2022
Merged

Exchanges: map rendering #40

merged 11 commits into from
Nov 18, 2022

Conversation

Kongkille
Copy link

@Kongkille Kongkille commented Nov 17, 2022

Issue

https://linear.app/electricitymaps/issue/ELE-1369/set-up-exchanges-layer

Description

This was a bit complicated. I tried to copy as much as possible but a few things seem to have been deprecated

  • BaseControl is no longer accessible, so I had to create custom layers in another way
  • OnViewPortChange also seems deprecated so we need to monitor isMoving on direct handlers such as onDrag end start etc. This is actually pretty nice, so we don't have the custom debounce logic.
  • Easing has been added so we need to turn it off with DragPan settings
  • A few other things that I had to change due to either me not understanding it, or the libraries having changed functionality somehow.

Performance is... ok. Similar to the current version. I had hoped that I could easily tweak some things and get good performance but the current approach just

I think part of it comes directly from the useExchangeArrows hook, so we could optimize that otherwise I think we need to go back to the whiteboard to get really good performance.

Preview

Screen.Recording.2022-11-17.at.18.35.02.mov

Copy link

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

LGTM

@Kongkille
Copy link
Author

LGTM

haha

@Kongkille
Copy link
Author

I can do the arrows in another PR if it's annoying to look at

Copy link

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

Works well, locally.

I find the custom layer wrapper stuff a bit confusing, not your code but the way the library is meant to be used. Would be great if we could go through the PR tomorrow so I can understand it a bit better and provide some more constructive feedback.


return (
<div
id={key}
Copy link

Choose a reason for hiding this comment

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

Could add className="select-none" to make it so arrows aren't draggable anymore

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I tried, it didn't work. Not sure how to get that working.

It's the same as in prod atm.

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Just some thoughts and questions. 🙂

web/src/features/map/Map.tsx Outdated Show resolved Hide resolved
web/src/features/map/Map.tsx Outdated Show resolved Hide resolved
web/src/features/exchanges/ExchangeArrow.tsx Outdated Show resolved Hide resolved
web/src/utils/scales.ts Show resolved Hide resolved
Markus Killendahl and others added 3 commits November 18, 2022 09:57
@Kongkille Kongkille mentioned this pull request Nov 18, 2022
@Kongkille
Copy link
Author

Works well, locally.

I find the custom layer wrapper stuff a bit confusing, not your code but the way the library is meant to be used. Would be great if we could go through the PR tomorrow so I can understand it a bit better and provide some more constructive feedback.

To be honest, I don't think I fully understand it either but we can try :)

Copy link

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

👍

@Kongkille Kongkille merged commit 33b18c0 into master Nov 18, 2022
@Kongkille Kongkille deleted the mk/add-exchanges branch November 18, 2022 10:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants