-
Notifications
You must be signed in to change notification settings - Fork 946
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
Enables history view for countryPanel #4220
Conversation
… 1 (#4208) * Add new helpers * Implement helpers * use helpers in timecontrols * Enable linting for formatting
… 2 (#4209) * Add new helpers * Implement helpers * use helpers in timecontrols * Enable timecontrols * timeaxis formats ticks depending on language and aggregate
… 3 (#4210) * Add new helpers * Implement helpers * use helpers in timecontrols * Enable timecontrols * timeaxis formats ticks depending on language and aggregate * Adds new title component * Uses new countryPanel. Unrelated, also disables the price graph for other aggregates * Updates english translations using the default option * Updates all languages with automatic script * Add en examples
… 4 (#4211) * Add new helpers * Implement helpers * use helpers in timecontrols * Enable timecontrols * timeaxis formats ticks depending on language and aggregate * Adds new title component * Uses new countryPanel. Unrelated, also disables the price graph for other aggregates * Updates english translations using the default option * Updates all languages with automatic script * uses v5 history when feature enabled * Ensure timeindex is handled correctly when switching between aggregates * Ensure correct start and end date for graphs * Disable capacity mention in tooltip for other aggregations * use constant
* Add new helpers * Implement helpers * use helpers in timecontrols * Enable timecontrols * timeaxis formats ticks depending on language and aggregate * Adds new title component * Uses new countryPanel. Unrelated, also disables the price graph for other aggregates * Updates english translations using the default option * Updates all languages with automatic script * uses v5 history when feature enabled * Ensure timeindex is handled correctly when switching between aggregates * Ensure correct start and end date for graphs * Disable capacity mention in tooltip for other aggregations * use constant * Use 0 as start index when switching aggregates to guarantee that it works * Ensure map updates when switching aggregates * add all formats * Make areagraph adjust ticks correctly * Make timeslider adjust correctly * Adjust time to range temporarily to match incoming data * Ensure proper timeaxis frequency * Uses v5 datetimes, and makes sure that state is refetched * lint
Just added some commits that resolves the most obvious issues To fix some inconsistencies with the timeaxis I had to use the central state datetimes, so I just enabled the v5/state for all aggregates as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! 🤩
I have some comments, some of which you may have planned to address later, but I've added them nonetheless to make sure we talk about it :)
const renderTick = (scale, val, idx, displayLive, lang) => { | ||
const shouldShowValue = idx % TICK_VALUE_FREQUENCY === 0; | ||
const renderTick = (scale, val, idx, displayLive, lang, selectedTimeAggregate) => { | ||
const shouldShowValue = idx % TIME_TO_TICK_FREQUENCY[selectedTimeAggregate.toUpperCase()] === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it's better to keep the object keys lowercase instead, so we don't have to do this? :)
web/src/helpers/constants.js
Outdated
const TIME_TO_RANGE = { | ||
HOURLY: 24, | ||
DAILY: 30, | ||
MONTHLY: 10, // Should be 12 - inconsistency with app-backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it only returning 10? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now removed this constant to instead rely on the length of the datetimes array returned from the backend
web/src/layout/timeController.jsx
Outdated
// TODO: set index to the max of the range of selected time aggregate | ||
dispatchApplication('selectedZoneTimeIndex', 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed from dispatchApplication('selectedZoneTimeIndex', TIME_TO_RANGE[aggregate.toUpperCase()] - 1);
?
It feels a bit wrong that it's starting a 0 and not the latest value in my opinion (e.g. switching from "today" to "yearly" it makes sense that it stays the same year instead of jumping to 2018)
@@ -1,7 +1,8 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most robust solution would be to have some sort of loading UI I am not sure how it would look with a spinner. Perhaps there is some skeleton UI solution?
@@ -1,7 +1,8 @@ | |||
import React from 'react'; | |||
import { useTranslation } from '../helpers/translation'; | |||
import styled, { css } from 'styled-components'; | |||
import { formatHourlyDate } from '../helpers/formatting'; | |||
import { formatDate, formatTimeRange } from '../helpers/formatting'; | |||
import { TIME } from '../helpers/constants'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by using datetimes from app-backend. Right now the app-backend state is a bit off, but will be fixed
@@ -4,6 +4,7 @@ import ReactMapGL, { NavigationControl, Source, Layer } from 'react-map-gl'; | |||
import { noop } from '../helpers/noop'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not related to this file, just adding it somewhere)
The production/consumption toggle no longer works - is this on purpose? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works for me?
It doesn't change the colors immediately, but neither does the current version
I'd also be happy to work on these comments together if you want, we can discuss it IRL tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
but a bit better than 0
…contrib into temp_main
Merges temporary main to master after a few sequential PRs