Skip to content
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

WIP wifi-menu #935

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP wifi-menu #935

wants to merge 1 commit into from

Conversation

TojikCZ
Copy link
Contributor

@TojikCZ TojikCZ commented Jan 30, 2024

No description provided.

@TojikCZ TojikCZ marked this pull request as draft January 30, 2024 12:30
@TojikCZ TojikCZ changed the title WIP WIP wifi-menu Jan 30, 2024
Copy link

@MichalKalita MichalKalita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 8 spaces in JS is a lot.
Better can be spaces, or 1 tab. And setup in editor how wide it should be.

All review commends are only "cosmetic" and should not change how the code works. So I am approving this PR.

It think it's good work for first Svelte usage in real application 🎉

@@ -0,0 +1,3817 @@
var __defProp = Object.defineProperty;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about workflow.
Standard way is not include build into source code in git.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused icon

Comment on lines +39 to +44
const update = { ...options };
update.headers = {
...update.headers,
"X-Instance-Fingerprint": instanceFingerprint
};
return update;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const update = { ...options };
update.headers = {
...update.headers,
"X-Instance-Fingerprint": instanceFingerprint
};
return update;
return {
...options,
headers: {
...options.headers,
"X-Instance-Fingerprint": instanceFingerprint
}
};

fetcher(requestUrl + "/wifi/api/hotspot_not_needed", {method: 'POST'});
}

const instanceFingerprint = document.getElementById("instance-fingerprint").value;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be unsafe, when JS is loaded before element exists.
It will be better to wrap this into simple function.

Comment on lines +11 to +15
const data = new URLSearchParams()
for (let field of formData) {
const [key, value] = field
data.append(key, value)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

form data can be used directly as input to URLSearchParams

https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams

const data = await response.json();
processConnectionInfo(data);
} catch (error) {
console.log(error);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use console.error, here and in line 99

<div class="container p-0" transition:slide>
<div class="row">
<div class="col-lg-4 col">
<ConnectForm ap={ {} } {connectionChange}/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of ap={ {} }

You can setup default value in ConnectForm component
export let ap = {};

Or make it working with undefined value

</div>
{#each aps as ap (ap.ssid)}
<!-- svelte-ignore a11y-no-static-element-interactions svelte-ignore a11y-click-events-have-key-events -->
<div class="row border border-white border-top-0 pt-2 pb-2" on:click|stopPropagation={() => {selectAp(ap);}} transition:slide>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<div class="row border border-white border-top-0 pt-2 pb-2" on:click|stopPropagation={() => {selectAp(ap);}} transition:slide>
<div class="row border border-white border-top-0 pt-2 pb-2" on:click|stopPropagation={() => selectAp(ap)} transition:slide>

Comment on lines +97 to +103
let potentialRedirectProbe;
for (const probeResult of receivedProbeResulte) {
if (probeResult.reachable && !probeResult.sameAsHost) {
potentialRedirectProbe = probeResult;
break;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

receivedProbeResulte typo, should be s on end

Suggested change
let potentialRedirectProbe;
for (const probeResult of receivedProbeResulte) {
if (probeResult.reachable && !probeResult.sameAsHost) {
potentialRedirectProbe = probeResult;
break;
}
}
let potentialRedirectProbe = receivedProbeResulte.find(r => r.reachable && !r.sameAsHost)

Comment on lines +115 to +121
let newAvailable = [];
for (const probeResult of receivedProbeResults) {
if (probeResult.ip && !probeResult.sameAsHost) {
newAvailable.push(probeResult.ip);
}
}
availableIps = newAvailable;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let newAvailable = [];
for (const probeResult of receivedProbeResults) {
if (probeResult.ip && !probeResult.sameAsHost) {
newAvailable.push(probeResult.ip);
}
}
availableIps = newAvailable;
availableIps = receivedProbeResults
.filter(r => probeResult.ip && !probeResult.sameAsHost)
.map(r => r.ip);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants