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

feat(web): handle historcal linking with basic navigation #7397

Merged

Conversation

tonypls
Copy link
Collaborator

@tonypls tonypls commented Nov 12, 2024

Issue

View historical data with hourly granularity

Description

Currently behind the feature flag historical-linking

This PR updates the state and zone requests to be able to query historical data at at hourly granularity and some basic navigation between time windows.

The navigation currently going back to the more recent utc 00:00 hour for the first click and subtracts 24 hours from there.

In the spirit of deploying early and often I'd like to release this behind the feature flag and get it in the hands of our test users asap for feedback.

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.

Some early comments, but the only thing I would require a change of is the url handling so it supports upper case as well and to use the Intl formatRange function.

web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/utils/formatting.ts Outdated Show resolved Hide resolved
web/src/utils/state/atoms.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getZone.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
web/src/api/getState.ts Outdated Show resolved Hide resolved
@cadeban
Copy link
Contributor

cadeban commented Nov 19, 2024

The code looks good and the functionality works as expected. 👌

Last thing - the direction buttons are disabled in non-hourly views. Do we want to hide them? Or maybe show a tooltip on hover explaining that this functionality is not enabled yet for aggregate views?
Screenshot 2024-11-19 at 09 51 31

EDIT: This can be a follow up conversation/PR

@tonypls
Copy link
Collaborator Author

tonypls commented Nov 19, 2024

The code looks good and the functionality works as expected. 👌

Last thing - the direction buttons are disabled in non-hourly views. Do we want to hide them? Or maybe show a tooltip on hover explaining that this functionality is not enabled yet for aggregate views? Screenshot 2024-11-19 at 09 51 31

EDIT: This can be a follow up conversation/PR

I think because it's behind a FF and we're going to add more features here we can get it out and improve in the next PRs.

Copy link
Contributor

@cadeban cadeban left a comment

Choose a reason for hiding this comment

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

lgtm!

@tonypls
Copy link
Collaborator Author

tonypls commented Nov 19, 2024

@cadeban I noticed some inconsistency in the navigation logic so I simplified it! Could ya please review the last commit?

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 suggestion.

Files not reviewed (6)
  • web/src/components/Time.tsx: Evaluated as low risk
  • web/src/features/time/HistoricalTimeHeader.tsx: Evaluated as low risk
  • web/src/features/time/TimeController.tsx: Evaluated as low risk
  • web/src/utils/formatting.ts: Evaluated as low risk
  • web/src/utils/state/atoms.ts: Evaluated as low risk
  • web/cypress/e2e/ranking.spec.ts: Evaluated as low risk

web/src/api/helpers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@cadeban cadeban left a comment

Choose a reason for hiding this comment

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

lgtm!

@tonypls
Copy link
Collaborator Author

tonypls commented Nov 20, 2024

Some early comments, but the only thing I would require a change of is the url handling so it supports upper case as well and to use the Intl formatRange function.

Hey Viktor, have your requested changes been addressed?

@tonypls tonypls enabled auto-merge (squash) November 20, 2024 13:34
@tonypls tonypls merged commit 30ea0ce into master Nov 20, 2024
23 checks passed
@tonypls tonypls deleted the tonyvanswet/avo-562-handling-incoming-historical-link-v3 branch November 20, 2024 13:55
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.

3 participants