-
Notifications
You must be signed in to change notification settings - Fork 131
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
Add network diagnostic tool to Help Box #391
Add network diagnostic tool to Help Box #391
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.
i'm still in the process of testing functionality, but i got what i expected with no changes to my network
i would like the diagnostic option moved into the Help section since this doesn't really feel like an Advanced App Setting
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.
let's move the Diagnostic component from Advanced App Settings to Help. put it right underneath <SupportBox>
on line 21
|
looks nice! updated screenshot https://imgur.com/a/aab8SU0 |
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.
Thanks for this PR. There are some changes I'd like to see before this gets merged.
done! |
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.
Getting there!
I've noticed a few common things:
- Prefer returning a
<div>
over returning react fragments, unless the structure of the DOM is important enough to require you to. - Prefer returning
null
over an empty react fragment<></>
. - Prefer inline text content
<div>text</div>
over wrapping in a string literal<div>{"text"}</div>
src/renderer/containers/Settings/NetworkDiagnostic/CgnatCommandSection.tsx
Outdated
Show resolved
Hide resolved
src/renderer/containers/Settings/NetworkDiagnostic/PortMappingSection.tsx
Outdated
Show resolved
Hide resolved
src/renderer/containers/Settings/NetworkDiagnostic/CgnatSection.tsx
Outdated
Show resolved
Hide resolved
src/renderer/containers/Settings/NetworkDiagnostic/NetworkDiagnosticsButton.tsx
Outdated
Show resolved
Hide resolved
all comments addressed! |
- add show/hide toggle for external ip address - add copy buttons - refine cgnat test failure ui
section tsx ternary -> arrow function
move diagnostic to help page
2.0.1 has broken `index.d.ts` so use 2.0.0 instead
This would make sense to explain what the network diagnostic would do before starting it, but doesn't make any sense to include once the user can see what the network diagnostic is doing
294e8d2
to
f9bf13d
Compare
This should make it easier for users and support volunteers to debug some network issues
screenshots: https://imgur.com/a/4iNsmec
In the future, I intend to hook up the port mapping test to a slippi dolphin port mapping option (IE. press a button to turn on slippi dolphin port mapping if port mapping is available)