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 focus argument in Terminal.prototype.open #645

Merged
merged 1 commit into from Apr 30, 2017

Conversation

parisk
Copy link
Contributor

@parisk parisk commented Apr 26, 2017

Fix #640

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 68.708% when pulling 35f746c on issue-#640-do-not-focus-on-open into 944da28 on master.

@parisk
Copy link
Contributor Author

parisk commented Apr 26, 2017

Opened up a documentation PR as well: xtermjs/xtermjs.org#20.

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2017

I don't think we should add an option to the API, isn't this the same as doing this?

term.open(el);
term.focus();

We could call it out in the release notes as a minor change, I don't see it breaking many applications though.

@parisk
Copy link
Contributor Author

parisk commented Apr 30, 2017

@Tyriar, this will break all applications that use xterm.js actually, since the focus will be elsewhere now.

Since this is changing the way it works and it is a user facing issue, I don't want to break the API in a minor release. It's a very simple piece of code, which can go away in the next major release.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Sounds good 😄

@parisk parisk merged commit 66f6b88 into master Apr 30, 2017
@parisk parisk deleted the issue-#640-do-not-focus-on-open branch April 30, 2017 22:07
@parisk parisk modified the milestone: 2.6.0 May 5, 2017
@coderaiser
Copy link
Contributor

Would be better to use options object instead of last boolean argument in v3. This way:

term.open(container, {
    focus: true
});

It is much cleaner and obvious then last boolean parameter.
What if there should be some other options, not only focus in a future?

@Tyriar
Copy link
Member

Tyriar commented May 8, 2017

A new object might cause confusion with Terminal(options). I would prefer if we removed the parameter outright in v3 so it's ignored.

@parisk
Copy link
Contributor Author

parisk commented May 9, 2017

I agree. Keeping this in options sounds quite better.

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

4 participants