Feature #13520
closedImprove Thermal Sensors Dashboard widget readability
100%
Description
https://github.com/pfsense/pfsense/pull/4616
The PR features a couple of changes.
1. improved upon the graph layout - changed the title to be above the progress bar and added a margin on bottom
Makes it easier to amke it out which bar belongs to which sensor.
2. Fixed a bug which would always build the rows in html instead of only updating them.
If you have a better solution than mine, please let me know.
3. "fixed" dark pfsense theme so that progress-bar matches the dark green color. Did not change other themes.
4. The second commit makes sure the widgets get updated as soon as possible after the user refreshes the page.
Without this, user would wait a couple of seconds before getting thermal sensors and other widgers' data.
A couple of before/after pictures regarding the first commit:
Files
Updated by Marcos M 2 months ago
- Subject changed from Update thermal sensor widget to Improve thermal widget readability
- Status changed from Pull Request Review to Feedback
- Target version set to 2.8.0
- Plus Target Version set to 24.08
- 4b0deb88ca437dea96d719388a4b9136b49123ee: This change affects all progress bars, not only the thermal widget. Instead of overriding the default theme with `!important`, use the same selector and leave the override behavior to the include order.
- b0c455aa5d3f1c3b1926dbcca0eacbe4f57e3ef6: Improve the thermal widget readability by adding a margin and sorting. The label is kept below the progress bar to match the rest of the GUI (e.g. the disk widget).
Other changes to be considered separately.
Updated by GChuf 6 2 months ago
First of all thank you for looking at the PR and your commits. I agree with all the changes you've made to my commits, I think it's better this way.
Regarding the rebuilding -
While looking at my commit it occurs to me that I probably didn't mention a very important detail: the variable firstTime
doesn't work correctly and the buildThermalSensorsDataGraph
function is the one that's executed on every ajax call. updateThermalSensorsDataGraph
never gets executed. This generates unnecessary load on clients and slows down the refresh.
Maybe it would be better to fix the issue with the variable first. This issue is present in some other widgets as well. I can't remember all the details since the PR is almost 2 years old.
I'd like to contribute more to the widget refresh system/widgets code, however it's hard to track things that are as old as this. I have some other commits written, but I'd like to ask if it's possible for you to review the code a bit sooner (a couple of weeks, a month). I know you're not obligated to do anything here, but I'd like to know if my commits will make it into the project and ultimately, if it's worth my time as well.
For example, I have a simple fix for the widget refresh system as I think the "refresh" doesn't work as intended at the moment (refresh interval is bugged, and widgets don't all update at the same time). I'll take a look at the code, clean it up and submit a PR. Maybe the No timeout on first load
commit can go there as well.
Let me know what you think of the html rebuilding and the firstTime
variable.
Thank you!
Updated by Jim Pingle about 2 months ago
- Subject changed from Improve thermal widget readability to Improve Thermal Sensors Dashboard widget readability
- Category changed from Web Interface to Dashboard
Updated by Jim Pingle about 1 month ago
- Plus Target Version changed from 24.08 to 24.11
Updated by Jim Pingle 10 days ago
- Status changed from Feedback to Closed
- Assignee set to Marcos M
- % Done changed from 0 to 100
The parts that were implemented appear to be OK, other parts can be moved to separate issues as needed if they are still desirable changes.