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

Added shorthand focus() method in generic Editor class #8346

Merged
merged 6 commits into from
Nov 3, 2020
Merged

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Oct 26, 2020

Suggested merge commit message (convention)

Feature (core): Introduced the focus() method in the base Editor class. Closes #714.


Additional information

The editor.focus() method is just a shorthand for editor.editing.view.focus().

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

the CI fails with:

WARN: 'Detected `console.warn()`:', 'There is no selection in any editable to focus.'
Chrome 86.0.4240.111 (Linux x86_64) Editor focus() should call view's focus() method FAILED
	Error: Detected `console.warn()`:
	    at console.<computed> [as warn] (webpack:///./build/.automated-tests/entry-point.js?:40:11)
	    at View.focus (webpack:///./packages/ckeditor5-engine/src/view/view.js?:438:33)
	    at Function.invoke (node_modules/sinon/pkg/sinon.js:1922:47)
	    at View.focus (node_modules/sinon/pkg/sinon.js:2178:26)
	    at TestEditor.focus (webpack:///./packages/ckeditor5-core/src/editor/editor.js?:19:4566)
	    at Context.eval (webpack:///./packages/ckeditor5-core/tests/editor/editor.js?:577:11)

https://travis-ci.org/github/ckeditor/ckeditor5/builds/738967351

@psmyrek psmyrek requested a review from mlewand October 27, 2020 10:30
@psmyrek
Copy link
Contributor Author

psmyrek commented Oct 27, 2020

CI build has been fixed by explicitly setting editor.editing.view.document.isFocused = true in the unit test, because the only thing this particular test is checking is that whether it calls the focus() method on the view - regardless of its real implementation details, which is already tested.

It's not needed to put implementation details in the method docs.
@mlewand
Copy link
Contributor

mlewand commented Oct 28, 2020

LGTM, just one last thing that we should do in this PR is to replace existing editor.editing.view.focus() calls with the new, nicer form in our codebase.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Please check the context of remaining changes.

@psmyrek psmyrek requested a review from mlewand October 28, 2020 10:26
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

LGTM, minor adjustment in one guide.

@mlewand mlewand merged commit dea8051 into master Nov 3, 2020
@mlewand mlewand deleted the i/714 branch November 3, 2020 08:52
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.

Implement editor.focus()
2 participants