-
Notifications
You must be signed in to change notification settings - Fork 8
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
Task 1 and Task 2 of GSOC'21 WebUI challenge completed #14
base: main
Are you sure you want to change the base?
Conversation
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.
Looks great so far, some notes:
- We are using yarn, so please remove the
package-lock.json
, also please remove any files not needed, like theyarn-error.log
- If one selects more than the available rows for the number of displayed rows, the table adds unnecessary padding to the bottom.
- The camera load should probably go into the
System Information
tab - Please graph the historic change of the load and not just the current value. Sorry if the description was unclear there.
- What is the reason for omitting the
BSSID
?
frontend/index.html
Outdated
content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" | ||
/> | ||
<meta http-equiv="X-UA-Compatible" content="ie=edge" /> | ||
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons" /> |
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.
This site should work without internet access, so this should be vendored or some other solution should be found.
What are these used for?
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.
Looks great so far, some notes:
1. We are using yarn, so please remove the `package-lock.json`, also please remove any files not needed, like the `yarn-error.log`
Required refinements done for this
2. If one selects more than the available rows for the number of displayed rows, the table adds unnecessary padding to the bottom.
Extra padding removed from material table
3. The camera load should probably go into theSystem Information
tab
Done
4. Please graph the historic change of the load and not just the current value. Sorry if the description was unclear there.
Got your point replaced bar chart with a multi line chart.
5. What is the reason for omitting theBSSID
?
Not getting that as output , I did'nt ommit intentionally.
package.json
Outdated
@@ -29,7 +33,7 @@ | |||
"morgan": "~1.9.1", | |||
"parcel": "^1.12.4", | |||
"prettier": "^1.19.1", | |||
"react": "^16.12.0", | |||
"react": "^16.8.0 || ^17.0.0", |
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.
What is the reason for this version constraint?
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.
Taken care in upcoming commits
frontend/routes/Wifi.jsx
Outdated
data={[...this.state.wifi_networks]} | ||
options={{ | ||
headerStyle: { | ||
backgroundColor: '#FA8756', |
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.
this should probably follow the theme color?
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.
Taken Care in upcoming commits
frontend/routes/Wifi.jsx
Outdated
}, | ||
}, | ||
|
||
{ title: 'MODE', field: 'MODE', sorting: false }, |
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.
Why not allow sorting after mode, channel and rate?
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.
Sorting functionality added for these also
frontend/routes/Wifi.jsx
Outdated
</table> | ||
); | ||
} | ||
// function Table(props) { |
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.
Please remove unnecessary comments.
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.
Taken care in upcoming commits
frontend/routes/Wifi.jsx
Outdated
@@ -13,41 +16,95 @@ export class Component extends React.Component { | |||
last_refresh: '', | |||
}; | |||
setInterval(() => { |
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.
It would be a good idea to cleanup the setInterval
for this one aswell.
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.
Taken care in upcoming commits
frontend/routes/Wifi.jsx
Outdated
sorting: false, | ||
render: rowData => { | ||
return rowData['SECURITY'].split(' ').map(tag => { | ||
return <Chip key={tag} label={tag} variant="outlined" />; |
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.
The Chips
could use a bit of spacing between them.
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.
Taken car in upcoming commits
frontend/util/Bar.js
Outdated
); | ||
}; | ||
|
||
const Bar = props => { |
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.
Why not export default Bar ...
?
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.
Bar.js removed , using line chart for showing the historic change of camera load
Also you might want to add the email used for the commits to your github profile, so github properly recognizes the commits as written by you. |
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.
Some notes:
- The sorting of the wifi network table only updates after the data is updated, not immediately
- The order of the table column is not saved and resets after the data is updated
- The column one selected for sorting the table is lost, when the data is updated
- The load graph looses its data after closing it
- Its not possible to pan or zoom the graph
@@ -2,33 +2,50 @@ import React, { useEffect, useState } from 'react'; | |||
|
|||
import { exec } from '../util/exec'; | |||
|
|||
import Bar from '../util/Bar'; | |||
import Chart from 'react-google-charts'; |
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.
This package seems to load a script from the internet, unfortunately this doesn't really work for this application, as this site has to work without internet access.
let [load, setLoad] = useState([0, 0, 0]); | ||
const [load, setLoad] = useState([ | ||
['Time Lapsed(In seconds)', '1 minute avg', '5 minute avg', '15 minute avg'], | ||
[0, 0, 0, 0], |
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.
Adding these makes all of the graphs start from zero, even if the load never was zero.
setLoad(oldLoad => { | ||
let loadArr = [ | ||
[ | ||
oldLoad[oldLoad.length - 1][0] + 1, |
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.
This should probably use the actual time as included from the uptime
output, if for some reason some of the requests get stalled and are then completed all at once.
return ( | ||
<div> | ||
<Modal |
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.
Is there a specific reason for making this a modal popup?
If not embedding the chart directly on the system configuration page seems better.
useEffect(() => { | ||
setInterval(() => { | ||
let intervalId = setInterval(function a() { | ||
exec('uptime').then(result => { | ||
let splitres = String(result) | ||
.trim() |
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.
Is this first trim
necessary?
Replaced material-table library with inbuilt table in material UI , sorting and ordering issues resolved |
Camera Load visualization :
Camera Load Widget in System information tab :