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

Spatial Aggregations V2 #4627

Merged
merged 14 commits into from
Oct 10, 2022
Merged

Conversation

tonypls
Copy link
Collaborator

@tonypls tonypls commented Sep 30, 2022

Issue

Add spatial aggregations to the map, this PR removes the feature flag and includes aggregated exchanges on the map

Description

Only countries with defined "subZoneNames" are aggregated. A list of exchanges to exclude from each the country and zone view is generated based off the exchange config when running update-world.js

Preview

@VIKTORVAV99
Copy link
Member

I'll add some more comments/questions later, I need to grab something to eat and test it out a little more first. 🙂

@VIKTORVAV99
Copy link
Member

I'd like to revisit URL pattern as I think I have a compromise that we are already using for language.
http://localhost:8080/zone/SE-SE4?aggregated=false is very declarative and a bit excessive now that we use a state toggle similar to the production/consumption toggle.

I propose we make it funktion the same way as the ?lang=en-GB parameter for example. In the way that it sets the correct state when included in the URL and then it's removed from the URL. I realize it will be harder to share the correct page this way but if we include a share button somewhere like suggested in #4216 it would make sense.

We could also include a URL parameter for consumption/production the same way.

What do you think?

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 a few notes, nothing major found so far that needs to be addressed yet.

But I'll review it continuously when changes are made. 👍🏼

web/src/hooks/layers.js Outdated Show resolved Hide resolved
web/src/hooks/layers.js Outdated Show resolved Hide resolved
web/src/hooks/layers.js Outdated Show resolved Hide resolved
web/src/layout/leftpanel/countrypanel/index.jsx Outdated Show resolved Hide resolved
web/src/layout/leftpanel/countrypanel/index.jsx Outdated Show resolved Hide resolved
@VIKTORVAV99 VIKTORVAV99 linked an issue Oct 7, 2022 that may be closed by this pull request
@tonypls tonypls marked this pull request as ready for review October 10, 2022 09:46
web/geo/validate.js Outdated Show resolved Hide resolved
web/geo/validate.js Outdated Show resolved Hide resolved
@tonypls tonypls changed the title Mostly aggregate exchanges Spatial Aggregations V2 Oct 10, 2022
@tonypls tonypls requested a review from Kongkille October 10, 2022 13:22
@tonypls tonypls merged commit d38a47b into master Oct 10, 2022
@tonypls tonypls deleted the tvs/spatial-aggregations-on-the-map-v2 branch October 10, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

merge bidding zones inside a single country
3 participants