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

Switch to xterm.js #25

Merged
merged 3 commits into from
Jul 19, 2017
Merged

Switch to xterm.js #25

merged 3 commits into from
Jul 19, 2017

Conversation

jwforres
Copy link
Contributor

@jwforres jwforres commented Jul 7, 2017

No description provided.

@jwforres
Copy link
Contributor Author

jwforres commented Jul 7, 2017

cc @spadgett

still need to figure out why it doesn't seem to be honoring cols correctly

@jwforres
Copy link
Contributor Author

xterm.js switches to a model that has a visible scrollbar when there is content in the scrollback, for the browsers that support it we should style the scrollbar colors to look better with the black background

terminal-updates

@jwforres
Copy link
Contributor Author

@stefwalter do you have concerns about the visible scrollbar in general #25 (comment)

There a lot of things about the xterm behavior (plus long term support) that seem better at this point, so I think we definitely want to switch to it for openshift but I don't want to break things for cockpit. If necessary we will fork container-terminal and use our own version based on xterm. Let me know.

@jwforres
Copy link
Contributor Author

cc @rhamilto or @sg00dwin this is the PR for the xterm changes, if you want to play with the scrollbar colors to find something that looks good

@rhamilto
Copy link
Contributor

cc @rhamilto or @sg00dwin this is the PR for the xterm changes, if you want to play with the scrollbar colors to find something that looks good

I'll take a look

@stefwalter
Copy link
Contributor

There a lot of things about the xterm behavior (plus long term support) that seem better at this point, so I think we definitely want to switch to it for openshift but I don't want to break things for cockpit. If necessary we will fork container-terminal and use our own version based on xterm. Let me know.

I think xterm is the right way forward. Lets not fork. We pull a specific older version of container-terminal, but would update to this soon.

@stefwalter
Copy link
Contributor

FYI @petervo

@jwforres
Copy link
Contributor Author

We'll cut a major version (2.0) when we release this since its a big underlying change

@jwforres
Copy link
Contributor Author

Seeing weird scrollback behavior with the keyboard in applications like vi and less, partially improved by setting applicationCursor:true remaining issue seems to only happen if you arrow down a couple times and then arrow ALL the way back to the top, then the scrollback buffer isnt getting cleared out. If you arrow back only part way then the scrollback seems to contain the content you would expect.

@stefwalter
Copy link
Contributor

Just wanted to note: After we merge this, we should bump the Y number of the package, as this is more than just a micro update. Thanks for the effort here.

@jwforres
Copy link
Contributor Author

The remaining issue is minor relative to the benefits we gain, its being tracked upstream at xtermjs/xterm.js#802 would propose merging this, @spadgett ?

@spadgett
Copy link
Contributor

+1 -- I don't think that issue should block us from going ahead. This fixes a lot more problems than it introduces

@jwforres jwforres changed the title [WIP] Switch to xterm.js Switch to xterm.js Jul 19, 2017
@jwforres jwforres merged commit 25fa3c2 into kubernetes-ui:master Jul 19, 2017
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.

4 participants