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

Adds state with atom and initial timecontroller UI #21

Merged
merged 13 commits into from
Nov 9, 2022
Merged

Conversation

Kongkille
Copy link

@Kongkille Kongkille commented Nov 7, 2022

Should be reviewed per commit.

Issue

https://linear.app/electricitymaps/issue/ELE-1330/set-up-jotai-and-document-usage

Description

To sync across URL, localstorage and sessionstorage, I created a new atom which is based upon atomWithStorage and the raw code from atomWithHash.

There is some UI attached to this PR which could have been split out, but I don't think it's as controversial as the files in state.

Preview

image

.prettierrc.js Outdated Show resolved Hide resolved
@Kongkille Kongkille marked this pull request as ready for review November 7, 2022 08:37
Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

Some comments ❤️
(Sorry if it's a lot, but that's what you get when making large PRs 😛)

web/src/utils/state/index.ts Outdated Show resolved Hide resolved
web/src/utils/state/atomWithCustomStorage.ts Show resolved Hide resolved
web/src/utils/state/index.ts Outdated Show resolved Hide resolved
web/package.json Outdated Show resolved Hide resolved
web/src/components/TimeAverageToggle.tsx Outdated Show resolved Hide resolved
web/src/components/TimeAverageToggle.tsx Show resolved Hide resolved
web/src/components/TimeSlider.tsx Outdated Show resolved Hide resolved
web/src/utils/state/atomWithCustomStorage.ts Show resolved Hide resolved
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 two small questions.

Mads got most of them already! 😅

web/src/components/TimeAverageToggle.tsx Show resolved Hide resolved
web/src/utils/state/index.ts Outdated Show resolved Hide resolved
@Kongkille
Copy link
Author

Added functionality so that when you change the url parameter everything else updates as well

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.

I can update the naming if you like

@@ -0,0 +1,16 @@
export enum TimeAverages {
Copy link

Choose a reason for hiding this comment

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

I think time period is the right naming here, an average implies a mean value i.e. the average time to run a mile is 5 minutes. Here's more definitions of "time periods" https://www.britannica.com/dictionary/eb/3000-words/topic/periods-of-time

Copy link
Author

Choose a reason for hiding this comment

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

We asked platform who suggested time averages
https://electricitymaps.slack.com/archives/C01PWQYKX2R/p1666948439485229

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

LGTM now :)

@Kongkille Kongkille merged commit 964de62 into master Nov 9, 2022
@Kongkille Kongkille deleted the mk/add-jotai branch November 9, 2022 12:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants