-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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: Add options for Chart period #752
Conversation
New method of storing data implemented. Feels like magic to me but apparently custom watchers is a thing. |
80575b5
to
542eb18
Compare
Fix: Fix callback, add toast on error Fix: Improve styling Fix: Restore default chart behavior Fix: Replace 1h with 3h draft only
Fix: Fix import error
542eb18
to
665bae0
Compare
Great demanding feature. |
Thanks! Was thinking if I need to write unit test for the API, but I guess we haven't set that up yet. |
I think this pull request do not require unit testing, as it is pretty straight forward. Only large charges like #518 and #752 need to come with unit tests. I updated the contributing guide recently: |
Could this setting be sticky? |
Yes please - this would be great. That and perhaps adding a "default" setting to the Settings - e.g. so we can default all items to say 24 hours. |
PR for saving the period is open, please discuss in #1007. |
Closes #223
The current behavior is unchanged. The new default will be "recent", since the last 100 beats doesn't really correspond to a specific time period. Currently available options are 3h, 6h, 24h, 1w. Since these are manually triggered when the user selects, I don't think performance will be a big concern.
Slight issue: Updating the period will cause the heartbeat bar animation to trigger, since the data structure is shared. I don't see any easy solution to this.
In the future we can make the default period configurable, but should the Chart component handle this? Or should the
LIMIT 100
be configurable?