-
Notifications
You must be signed in to change notification settings - Fork 55
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 support for various transports, and mechanism to collect connectivity report #137
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.
Do you mind editing the PR title to make it more specific? Something like "add transport support to connectivity test app"
// Observations | ||
Time time.Time `json:"time"` | ||
DurationMs int64 `json:"durationMs"` | ||
Error *ConnectivityTestError `json:"error"` | ||
} | ||
|
||
func (r ConnectivityTestResult) IsSuccess() bool { | ||
if r.Error == nil { |
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 just return r.Error == nil
?
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 is fixed
x/examples/outline-connectivity-app/shared_frontend/pages/connectivity_test/index.ts
Outdated
Show resolved
Hide resolved
.popup { | ||
display: none; | ||
position: absolute; | ||
z-index: 1000; /* A high value to ensure it's on top */ |
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.
Yep, this is why I recommended looking into the popover
attribute. High z-indexes are a pretty major CSS code smell and are not scalable. Check it out: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover
If you don't want to use popover
, I can guide you to avoiding high z-indices, but it will take some iterations.
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.
@daniellacosse Thanks for the feedback. I got popover
working on browser and iOS and desktop app but on Android it does not function as intended. Please see the screen shots below for Android and iOS. I suspect the WebView on Android (emulator pixel 7, API level 33) maybe using a Chrome version older than 114 which does not support popover
. I am still investigating that theory. Please also let me know if you have any suggestions on how to best position the popover
box on the screen.
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.
You can use feature detection to hide the elements when popovers aren't supported.
With respect to positioning, I think you'll need the wrap the target and the popover in an element that's positioned relatively. Then position the popover element absolutely.
Alternatively we could just center the popover in the page and add a backdrop. That avoids having to write Javascript do deal with the intersection logic.
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 managed to get popover
working on all platforms.
The issue on Android was due to the fact that Capacitor uses WebView which is not automatically updated on Emulator. Once I logged into Google Play on Emulator and upgrade WebView manually it fixed the issue. I think this should work fine on most recent Android devices but If the device runs a very old version of Android API / WebView, the popover element won't work as expected.
Also, I observed that on iOS tapping outside of the popover
window does not hide it as other platforms. This is not a big deal since I added a close button x
to the popover window to offer an alternative to close it down.
@@ -598,22 +600,24 @@ export class ConnectivityTestPage extends LitElement { | |||
// TODO: move language definitions to a centralized place | |||
return html`<main dir="${this.locale === "fa-IR" ? "rtl" : "ltr"}"> | |||
<header class=${this.platform?.operatingSystem === OperatingSystem.IOS ? "header--ios" : "header"}> | |||
<h1 class="header-text">${msg("Outline Connectivity Test")}</h1> | |||
<h1 class="header-text">${msg("Transport Tester")}</h1> |
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.
If we're changing the title of the app, we'll need to change the folder name as well.
Why not Outline SDK Transport Tester
?
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 idea behind the name change was that with latest changes users can now test any of the supported transports in the SDK addition to ss
/outline. Maybe we can drop SDK and call it Outline Transport Tester
?
We can even keep the previous name. I think it will better to keep the title short.
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 drop "Outline", since this is not an official Outline product. "Connectivity Tester" is probably more user-friendly than "Transport Tester".
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.
Are we sure we want to drop "Outline"? I know it's not an official product, but a) it's free advertising and b) people will want to know the tool they're using.
I'm okay with shortening it to Connectivity Tester if we have a link somewhere explaining what the SDK is. Keeps the name short but serves the same goal of driving people to the SDK.
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.
Good point. Perhaps we add a line somewhere, as a subtitle or perhaps a footer, saying Powered by the Outline SDK? We could link to it too.
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 looked through the branding guideline and used the Powered By Outline
SVG logo at the bottom of the page. This way we can keep the title short and still attribute to Outline SDK. I am fine with either Transport Tester or Connectivity Tester for the title, but I think the former rhymes better :)
converting to draft as it needs rebase & some refactoring |
This PR introduces `tlsfrag:<len>` to the config format.
Co-authored-by: Vinicius Fortuna <[email protected]>
Co-authored-by: Vinicius Fortuna <[email protected]>
Co-authored-by: Vinicius Fortuna <[email protected]>
Explain what can be done with the SDK, expand integration documentation and create command line tools section.
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.7.0 to 0.17.0. - [Commits](golang/crypto@v0.7.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.14.0 to 0.17.0. - [Commits](golang/crypto@v0.14.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.14.0 to 0.17.0. - [Commits](golang/crypto@v0.14.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.10.0 to 0.17.0. - [Commits](golang/net@v0.10.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
Let me know if you'd like more detail.
This reverts commit e793764.
I had to rebase and refactor this PR and it ended up getting a bit messy. I decided to create a new branch off of main and related cherry pick commits into it. The result is a new PR #170. I am going to close this PR as a result. |
Connectivity Checker GUI Example App is extended to offer report collecting to a remote destination. One simple approach to set up a remote collector using Google Spreadsheet is discussed here.
CHANGELOG:
split:5|split:10|ss://[email protected]:65496
TODO: