nsqadmin: clean up client-host display #632
Conversation
|
|
||
| var port = remote_address.split(":").pop() | ||
| v['hostname_port'] = hostname + ":" + port; | ||
| }) |
There was a problem hiding this comment.
I don't know enough about when the clients attribute would change and what values we might expect client_id, hostname, and remote_address to be to review the logic of this code. I might tweak the naming a bit to make what this method is doing a little more explicit. For example, maybe something like setHostnamePort is better?
Besides that, it's mostly just being consistent with style (e.g. indentation, single quotes, missing semicolons, spaces between function() and {).
|
Hmm i realized i probably have some work i should also add in here to cleanup the display of the same fields on the nodes page as well. |
|
nodes page updated. Any feedback @mreiferson? |
| initialize: function() { | ||
| this.on('change:clients', this.calculateHostnamePort); | ||
| }, | ||
| calculateHostnamePort: function() { |
There was a problem hiding this comment.
vertical whitespace 👮 (and between functions)
|
got a screenshot with how this ends up looking by any chance? |
jehiah
force-pushed
the
nsqadmin_client_id_632
branch
from
a2ec788 to
6efd1f2
Compare
|
👍 |
nsqadmin: clean up client-host display
Prior to #323 the "Client Host" column in nsqadmin displayed the hostname appended by the port as taken from the remote-address. That PR switched to always show client_id and remote_address. It was unclear though the ideal relationship between Hostname, Name, Short_id and Client ID (see #298).
This clarifies the display to always show hostname + port (from remote_address), along with a hover state that shows the remote_address. Client ID will also be displayed when it's not equal to the hostname or a short version of the hostname.
RFR @mreiferson @rolyatmax