-
Notifications
You must be signed in to change notification settings - Fork 492
Added network scanning to config portal. #223
Conversation
…ted, unsupported state.
select = document.getElementById('networks'); | ||
select.innerHtml = ''; | ||
networks.sort(function(a, b) { | ||
return b.rssi - a.rssi; |
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.
maybe we should choose a more predictable sorting to avoid the list from being re-ordered on every scan.
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 seems like the sorting that would be most desirable, you get the strongest networks first. This is how OSX, Android, iOS, Windows, ChromeOS, etc.. sort.
The appeal of this sorting is that the most relevant networks are most likely to be on top. For example when I used to live in downtown LA I had roughly 20 networks I could see at a time but my home network was always the first or second because it was the strongest in my house.
</script> | ||
</head> | ||
<body> | ||
<div>Host: <input id='host'></div> | ||
<div>Auth: <input id='auth'></div> | ||
<div>Path: <input id='path'></div> | ||
<div>Wifi SSID: <input id='ssid'></div> | ||
<div>Wifi SSID: <input id='ssid'><button onclick='scan();'>scan</button></div> |
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.
did you consider attaching the onclick from the js and keeping the js free?
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.
not really, what would the advantage be?
Seems like more work, we would need a window.onload handler (might be nice to put the random "fetch_config()" call at the end of the document there too. then you would need to add an id here, fetch this button and add the onload, or just put it inline here. This seems cleaner.
If we had a strong reason for separation, like if the JS was loaded from another file and we wanted to play with loading different JS for the same html or something like that but as this is a small file with everything inline it seems to just stick with that and mix it all together. I could be convinced otherwise though.
option.text = network.ssid + ' (' + network.rssi + ')'; | ||
select.add(option); | ||
}); | ||
select.style.display='inline'; |
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.
something you could consider is setting a .class with the state ('scanning', 'selection', 'connected') on a parent div for the ui, and having some css that configure the visibility of all elements for each one instead of having the js show/hide 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.
I am not sure I follow what you are proposing. It may just be beyond my CSS knowledge. The UI is very much in a "working but unstyled" state. I was planning on going back at the end and making things look good, add some CSS to try to match the feel of the firebase console, stuff like that but if someone wanted to jump in and start styling it now I wouldn't complain :)
Since all comments are suggested improvements and not fixes I am going to go ahead and merge. Feel free to keep commenting though and we can make the changes in followup PRs. |
Use the ESP network module to allow the user to scan for networks.