-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add binary chart #2094
Add binary chart #2094
Conversation
#2747 Bundle Size — 10.22MiB (+0.24%).8e4932a(current) vs b65b18c master#2744(baseline) Warning Bundle contains 3 duplicate packages – View duplicate packages Bundle metrics
|
Current #2747 |
Baseline #2744 |
|
---|---|---|
Initial JS | 5.53MiB (+0.44% ) |
5.5MiB |
Initial CSS | 304.63KiB (+0.09% ) |
304.37KiB |
Cache Invalidation | 56.77% |
56.69% |
Chunks | 51 |
51 |
Assets | 171 |
171 |
Modules | 1492 (+0.13% ) |
1490 |
Duplicate Modules | 21 |
21 |
Duplicate Code | 0.83% (-1.19% ) |
0.84% |
Packages | 124 |
124 |
Duplicate Packages | 3 |
3 |
Bundle size by type 2 changes
2 regressions
Current #2747 |
Baseline #2744 |
|
---|---|---|
JS | 7.31MiB (+0.33% ) |
7.29MiB |
IMG | 2.48MiB |
2.48MiB |
CSS | 321.42KiB (+0.08% ) |
321.16KiB |
Fonts | 93.55KiB |
93.55KiB |
Other | 17.62KiB |
17.62KiB |
HTML | 13.58KiB |
13.58KiB |
Bundle analysis report Branch Terdious:add-binary-chart Project dashboard
Generated by RelativeCI Documentation Report issue
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2094 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 867 867
Lines 14182 14187 +5
=======================================
+ Hits 13967 13972 +5
Misses 215 215 ☔ View full report in Codecov by Sentry. |
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 created a device with 2 states in DB: opened at 3:32 in the morning, then closed at 7:32 in the morning :
Then, I open Gladys and create a binary chart for this device :
When I display the dashboard, the UI completely freezes and uses 100% CPU. I'm unable to do anything on the UI.
IMO there is something in the front that loops too much and blocks the JS
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.
Hey @Terdious ! Thanks for the changes :)
I have a few bugs when trying it:
- Text is inside the "bar" :
- I get a freeze when saving the dashboard with a binary chart on a power plug
Here is the error :
The payload :
- If I select a binary device, then unselect it, then select a non-binary device, I get a weird state where the chart type is unknown :
Before:
After :
- When I'm using Gladys in English, the text inside the popup is in french:
- On mobile, the popup is outside of the screen :
Thanks for this PR 🙂
…inary device, get a weird state where the chart type is unknown
I'm sorry but I tried to reproduce and despite zooming in / out changing resolution, etc. I never managed to get your result. On the other hand, before my previous modification I had the same problem, but I corrected it before your review
I added a
Corrected
Corrected
Corrected. Display top left box |
@Terdious Thanks for the fixes! Here is the payload that causes the freeze:
The response is 422 :
It's just because I didn't select the "chart type" ^^ It's maybe not related to this PR, but I wonder if we could make a fix to avoid freezing in this situation... I have another feedback, now the date in english is not correct: |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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.
Thanks for the changes! Indeed, for the auto refresh it's working on my side too. I think it's because the bar was so small I didn't see it, but in the request tab I can see the auto refresh working :) |
e430ee7
to
ead8043
Compare
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.
Hey! A few feedbacks on the code, nothing serious :)
Otherwise, tested locally and it works great! Great work 👏
@@ -0,0 +1,215 @@ | |||
const addYAxisStyles = () => { | |||
const yAxisLabel = document.querySelectorAll('.apexcharts-yaxis-label'); |
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.
Where is this coming from?
ApexChart documentation?
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.
No, I found this solution on stackoverflow and on apexcharts github
server/test/lib/device/device.getDeviceFeaturesAggregates.test.js
Outdated
Show resolved
Hide resolved
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.
Good for me now! Thanks for all the good work :)
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm test
on both front)npm run eslint
on both front)npm run prettier
on both front)npm run compare-translations
on front)front/src/config/demo.js
) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Taken from the original PR: #Binary graph #1948
Thanks to @callemand for all the work done