-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support room dir 3rd party network filtering #2747
Conversation
As per matrix-org/synapse#1676 The existing local config system still exists and is used for remote home server directories (since /thirdparty/protocols doesn't support listing remote home servers, and also because people are using it).
Looks like you're hitting the right APIs. |
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.
Looks like javascript to me. I struggle to get my head around this code. Not being able to use half of it on my own HS doesn't help.
Probably fine, a couple of nits
} | ||
} | ||
|
||
return options; | ||
} | ||
|
||
_makeMenuOptionFromProtocolInstance(server, protocol, instance, wire_onclick) { |
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.
is wire_onclick ever false? why is it called that anyway?
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.
Yeah, as we only want to wire the onclick handler when it's used as a selectable menu option, rather than the currently selected menu option (ie. in the unexpanded dropdown).
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.
right, but it doesn't seem to be set in either of the places it's called.
and oh, wire is a verb. How about handleClicks
or something? (don't we do camelCase rather than underscores?)
or just have an onProtocolInstanceClicked
parameter, and pass in this.onMenuOptionClickProtocolInstance
or null
.
@@ -202,6 +253,24 @@ export default class NetworkDropdown extends React.Component { | |||
</div>; | |||
} | |||
|
|||
protocolNameForInstanceId(instance_id) { |
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.
needs _ if it's private
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.
Done
@@ -131,6 +131,11 @@ module.exports = React.createClass({ | |||
if (my_server != MatrixClientPeg.getHomeServerName()) { | |||
opts.server = my_server; | |||
} | |||
if (this.state.instance_id) { |
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.
add instance_id to getInitialState?
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.
Done
} | ||
} | ||
|
||
return options; | ||
} | ||
|
||
_makeMenuOptionFromProtocolInstance(server, protocol, instance, wire_onclick) { |
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.
right, but it doesn't seem to be set in either of the places it's called.
and oh, wire is a verb. How about handleClicks
or something? (don't we do camelCase rather than underscores?)
or just have an onProtocolInstanceClicked
parameter, and pass in this.onMenuOptionClickProtocolInstance
or null
.
}); | ||
this.props.onOptionChange(server, network); | ||
} | ||
|
||
onMenuOptionClickProtocolInstance(server, instance_id, ev) { |
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.
ev
seems redundant
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.
done
and actually pass handleClicks false as appropriate
As per matrix-org/synapse#1676
The existing local config system still exists and is used for
remote home server directories (since /thirdparty/protocols
doesn't support listing remote home servers, and also because
people are using it).