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

Displays the region participants are connected to #3451

Merged
merged 5 commits into from Sep 17, 2018

Conversation

bgrozev
Copy link
Member

@bgrozev bgrozev commented Sep 14, 2018

and the number of (media) servers in the stats panels.

@bgrozev
Copy link
Member Author

bgrozev commented Sep 14, 2018

screen shot 2018-09-14 at 16 45 44

screen shot 2018-09-14 at 16 45 36

@@ -430,6 +433,7 @@ class ConnectionIndicator extends Component {
packetLoss = { packetLoss }
region = { region }
resolution = { resolution }
serverRegion = { serverRegion }
shouldShowMore = { this.state.showMoreStats }
Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, maybe at this point, not saying for this PR, ConnectionStatsTable should just take in the stats object itself or we use a spread operator to pass in all stat values ({ ...this.state.stats })or have ConnectionStatsTable listen instead of just ConnectionIndicator.

* The number of bridges (aka media servers) currently used in the
* conference.
*/
bridgeCount: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

The count is really a string and not a number?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, nice attention to detail. I had it as a number, but there was an error at some point and I took the lazy way out :)
I'll make it a number and make sure that that's what the lib emits.

let str = serverRegion;

if (!serverRegion) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you prefer showing N/A over not showing the row?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to hide it because of deployments where regions are not configured. There it would show 'N/A' permanently (while for the rest of the stats it is reasonable to expect that they will appear once we get the data).

@@ -86,6 +92,11 @@ class ConnectionStatsTable extends Component {
*/
resolution: PropTypes.object,

/**
* The region of the media server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there needs to be a distinction made between region and serverRegion, at least in the docs.

lang/main.json Outdated
"bitrate": "Bitrate:",
"bridge_count": "Server count: ",
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think maybe not using snake_case is okay. At some point the other snake_case strings should probably be changed to camelCase.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, ok, I didn't know that.

@@ -373,8 +435,10 @@ class ConnectionStatsTable extends Component {
{ this._renderBitrate() }
{ this._renderPacketLoss() }
{ isRemoteVideo ? this._renderE2eRtt() : null }
{ isRemoteVideo ? this._renderRegion() : null }
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall you asking me about this. I think you can also do isRemoteVideo && this._renderRegion() and get the same result, but either words for me.

virtuacoplenny
virtuacoplenny previously approved these changes Sep 15, 2018
@bgrozev bgrozev merged commit d051d34 into jitsi:master Sep 17, 2018
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.

None yet

2 participants