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

Migrate ZoneList component to React #2145

Merged
merged 8 commits into from Jan 9, 2020
Merged

Migrate ZoneList component to React #2145

merged 8 commits into from Jan 9, 2020

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jan 8, 2020

Part of #1681 (comment).

Notes

  • When testing locally and switching the toggle to production, the zone list will become empty as there is no production data is generated for the local runs. This should actually be a correct behavior as the zone list should presumably only display zones for which some data exists. In the old ZoneList component, the list wouldn't really get updated when switching the toggle which is probably a bug.

@fbarl fbarl mentioned this pull request Jan 8, 2020
@fbarl fbarl changed the title [WIP] Migrate ZoneList component to React Migrate ZoneList component to React Jan 8, 2020
@fbarl fbarl marked this pull request as ready for review January 8, 2020 22:52
Copy link
Member

@corradio corradio left a comment

Choose a reason for hiding this comment

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

Superb!
I didn't look in the smallest details, so therefore, I would ask you to test thoroughly that the interaction still works.
It's important as well to test on mobile (I think it's enough if you test it from your phone, or from a browser in responsive mode).
I'll let you merge once you've tested.

@fbarl
Copy link
Contributor Author

fbarl commented Jan 9, 2020

I just ran through all the interactions tests I could think of and it seems to work fine:

Web

  • Rendering of the zone list
  • Clicking on the zones
  • Keyboard interactions (up/down/enter/a-z)
  • Search bar filtering
  • Clicking on the map selects the zone

Mobile

  • Rendering of the zone list
  • Clicking on the zones
  • Search bar filtering
  • Clicking on the map selects the zone

The only bit of code I wanted to be careful about is the map centering event observer I added in this PR, but the map interactions seem to work fine as well!

observe(state => state.application.selectedZoneName, (selectedZoneName, state) => {
  if (selectedZoneName) {
    centerOnZoneName(state, selectedZoneName, 4);
  }
});

Note: For mobile testing, I actually used Chrome in responsive mode as I couldn't get ngrok http 8000 to work from my mobile due to too many connections error :/ I wonder how you normally test from your phone, couldn't find anything in the docs 🤔

@corradio
Copy link
Member

corradio commented Jan 9, 2020

That's perfect!

@corradio corradio merged commit ae11f62 into electricitymaps:master Jan 9, 2020
@fbarl fbarl deleted the 1681-migrate-zone-list branch January 9, 2020 13:44
con-cat pushed a commit to con-cat/electricitymap-contrib that referenced this pull request May 18, 2021
* Started writing ZoneList React component; DOM rendering works

* Keyboard navigation through the zones list

* Mouse click action for the zones

* Make zones search bar work

* Remove old ZoneList component

* Rename zonelistreact -> zonelist

* Extract common getCo2Scale helper function

* Fix linting errors
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.

None yet

2 participants