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

Add control status icon to Terminal header #2087

Merged
merged 1 commit into from
Dec 20, 2016

Conversation

jpellizzari
Copy link
Contributor

@jpellizzari jpellizzari commented Dec 12, 2016

Implements #1982

This change will indicate the control status of the terminal. For example, if you are using the exec control a terminal icon will appear in the upper left (same for the 'attach' command):
screen shot 2016-12-12 at 1 49 48 pm

The icon is tightly coupled to the human name of the controls: "Attach" and "Exec shell". This will break if we ever change those values. If there is a different value to lookup (like an ID) that might be better.

@foot could you review please?

@jpellizzari jpellizzari self-assigned this Dec 12, 2016
@jpellizzari jpellizzari requested a review from foot December 12, 2016 22:24
@jpellizzari jpellizzari force-pushed the 1982-control-indicator branch 2 times, most recently from a27319c to 0ad48d8 Compare December 12, 2016 22:27
@jpellizzari jpellizzari changed the title Added control status icon to Terminal header Add control status icon to Terminal header Dec 12, 2016
@foot
Copy link
Contributor

foot commented Dec 13, 2016

Slick! Like the design.

raw: true|false aligning w/ the icons is a bit co-incidental, though its a bummer to add more state it would be nice to record the icon of the control that created that pipe and use that explicitly. If you save it into the controlPipe object next to the raw status it should be de/serialized into the url state for reloads and what not.

return (
<span
style={{marginRight: '8px', width: '14px'}}
className={classNames('fa', {[`fa-${icon}`]: icon})}

This comment was marked as abuse.

@jpellizzari jpellizzari force-pushed the 1982-control-indicator branch from 0ad48d8 to 2b83ebd Compare December 13, 2016 18:04
@jpellizzari
Copy link
Contributor Author

@foot Updated to use the 'icon' field of the control. Thanks, that's much better.

@jpellizzari jpellizzari assigned foot and unassigned jpellizzari Dec 19, 2016
@foot
Copy link
Contributor

foot commented Dec 20, 2016

Nice.

There's a bit of awkward implicitness in how we do the url state stuff.

As the entire control is being saved into the controlPipe you get a bit of unused info ( controlPipe.control.{id,probeId,human,nodeId}) up in there:

{
    "controlPipe": {
        "control": {
            "human": "Exec shell",
            "icon": "fa-terminal",
            "id": "docker_exec_container",
            "nodeId": "6229d32d614b8f7bc0ee51791ddf197de5a9d3c950a31e441082ab9528855f2c;<container>",
            "probeId": "392656d14781da1e",
            "rank": 2
        },
        "id": "pipe-5735362070544949278",
        "nodeId": "6229d32d614b8f7bc0ee51791ddf197de5a9d3c950a31e441082ab9528855f2c;<container>",
        "raw": true,
        "resizeTtyControl": {
            "id": "docker_resize_exec_tty",
            "nodeId": "6229d32d614b8f7bc0ee51791ddf197de5a9d3c950a31e441082ab9528855f2c;<container>",
            "probeId": "392656d14781da1e"
        }
    },
    "gridSortedBy": "label",
    "gridSortedDesc": false,
    "nodeDetails": [
        {
            "id": "6229d32d614b8f7bc0ee51791ddf197de5a9d3c950a31e441082ab9528855f2c;<container>",
            "label": "weavescope-app",
            "topologyId": "containers"
        }
    ],
    "pinnedMetricType": null,
    "pinnedSearches": [],
    "searchQuery": "",
    "selectedNodeId": "6229d32d614b8f7bc0ee51791ddf197de5a9d3c950a31e441082ab9528855f2c;<container>",
    "topologyId": "containers",
    "topologyOptions": {
        "containers": {
            "container-label-filters-group": "all",
            "pseudo": "hide",
            "stopped": "both",
            "system": "both"
        },
        "processes": {
            "unconnected": "hide"
        }
    },
    "topologyViewMode": "topo"
}

But actually its not the worst thing in the world, we might need that info in the future. We have another ticket #1727 for tightening up the url state.

LGTM!

@jpellizzari jpellizzari merged commit 32bb7c3 into master Dec 20, 2016
@jpellizzari jpellizzari deleted the 1982-control-indicator branch December 20, 2016 15:39
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.

2 participants