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

Feature to improve stats app charts #347

Merged

Conversation

forbesus
Copy link
Contributor

@forbesus forbesus commented Sep 2, 2024

refs #298

@forbesus
Copy link
Contributor Author

forbesus commented Sep 2, 2024

hello @baktun14
I have done #298 would you check this pr?

@baktun14
Copy link
Contributor

baktun14 commented Sep 3, 2024

Hey @forbesus I just tested and it works great! I have a few modifications requirements:

  • Would it possible to remove the TV logo?
  • When you switch between 7d 1M ALL, the zoom works but it only includes the data of that range so when you zoom out in 7d/1M, it doesn't really work. There's some logic that slices the data coming from props rangedData
  • Would it possible to have less padding by default at the top and bottom of the chart?
    image
  • Would it be possible to have the same unit as the current value in the right axis labels?
    image
    image
  • Would it be possible to decrease the line width a tiny bit?

Thanks for your contribution!

"lucide-react": "^0.292.0",
"moment": "^2.30.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove moment and use data-fns, that's the library we use in other projects for time based functions.

? rangedData.map(_snapshot => (
{
time: moment.utc(_snapshot.date).format('YYYY-MM-DD'),
value: _snapshot.value
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a unitFn called in the previous code y: roundDecimal(snapshotMetadata.unitFn(_snapshot.value).value) which is probably why the values don't match.

@forbesus
Copy link
Contributor Author

forbesus commented Sep 5, 2024

Hello @baktun14 i fixed and completed
Would you check again?

@baktun14
Copy link
Contributor

baktun14 commented Sep 5, 2024

@forbesus Great, I will check today.

@baktun14
Copy link
Contributor

baktun14 commented Sep 9, 2024

@forbesus There's a bug when changing the time range, it duplicates the graph. I'm investigating it.
image

@forbesus
Copy link
Contributor Author

forbesus commented Sep 9, 2024

Hello @baktun14 I checked it but I have a question
Did you check it after removing browser cache?

@baktun14
Copy link
Contributor

baktun14 commented Sep 9, 2024

Hello @baktun14 I checked it but I have a question Did you check it after removing browser cache?

It doesn't change the behavior for me

@forbesus
Copy link
Contributor Author

forbesus commented Sep 9, 2024

Hello @baktun14
I fixed that problem
Would you check again?

@baktun14
Copy link
Contributor

baktun14 commented Sep 9, 2024

@forbesus It works great!

@baktun14
Copy link
Contributor

baktun14 commented Sep 9, 2024

Great work @forbesus ! I fixed a resize issue and theme issue, but otherwise all is good to merge.

@forbesus
Copy link
Contributor Author

forbesus commented Sep 9, 2024

Thank you @baktun14

@baktun14 baktun14 merged commit 4610359 into akash-network:main Sep 9, 2024
5 checks passed
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.

2 participants