Updating to latest deps and ensuring compatibility with future API changes#135
Updating to latest deps and ensuring compatibility with future API changes#135yinzara wants to merge 9 commits intoimprov-wifi:mainfrom
Conversation
This required reformatting all files in the repo which caused some changes to serial.ts and provision.ts that were non-functional. This dialog now follow modern Material Design guidelines
yinzara
changed the title
There was a problem hiding this comment.
All the changes to this file were because of the update to prettier.
There was a problem hiding this comment.
All deps updated to latest so this PR can close all those other PRs that bump dependency versions from years ago.
|
After reviews with @balloob here are the final screenshots |
| > | ||
| <md-dialog open @close=${this._handleClose}> | ||
| <div slot="headline">${heading}</div> | ||
| <form slot="content" id="improv-form" method="dialog">${content}</form> |
There was a problem hiding this comment.
We always render a form ? Why is that not part of the actual form
There was a problem hiding this comment.
Yeah it's how it does form validation properly.
There was a problem hiding this comment.
Right, but shouldn't we render this only when we are rendering the actual form, not the other options?
src/serial-provision-dialog.ts
Outdated
| ${ | ||
| // Show input box if command not supported or "Join Other" selected | ||
| !selectedSsid | ||
| this._ssids === null || this._selectedSsid === null |
There was a problem hiding this comment.
We will now show a text box when the SSIDs are still loading? Can we just show a loading spinner and wait up to 5s for first SSIDs to come in?
There was a problem hiding this comment.
This is for when Scanning Wifi is not supported as well. I can also fix it to not show until there are at least some SSIDs retrieved when scan wifi is supported. Don't need the timeout.
There was a problem hiding this comment.
I often see that right after rebooting, it can take a bit of time before refresh returns any SSIDs. Probably starting up still.
There is ofc always the option that there are no SSIDs 😬
| href=${this._client!.nextUrl} | ||
| class="has-button" | ||
| > | ||
| <is-button label="Visit Device"></is-button> |
There was a problem hiding this comment.
This was a very important button. It allowed users to re-connect later to the device and visit the device without setting it up again.
There was a problem hiding this comment.
We can put this in the device info box.
|
New screenshots after review. 
|
|
Shown with an error. Is there any other formatting we want on the error message or is this good enough? |
|
let's make the background white, otherwise I think it's good to go. Sorry for the delay. Vegas and stuff 😅 |
|
2 participants
Minor change to support allowing more than YES/NO security attributes was in first commit.
2nd commit updates to latest deps and sets the design to follow modern material design guidelines.
High level the changes are as follows:
Screenshots are from local dev environment
