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

Move creation API to the editor directly #1242

Merged
merged 5 commits into from
Nov 28, 2017
Merged

Move creation API to the editor directly #1242

merged 5 commits into from
Nov 28, 2017

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 27, 2017

What is the purpose of this pull request?

Task: API refactoring

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

Closes #1237.

* Set of instance-specific public APIs exposed by the [Balloon Toolbar](https://ckeditor.com/cke4/addon/balloontoolbar) plugin.
* Balloon Toolbar manager for a given editor instance. It ensures that there's only one toolbar visible at a time.
*
* It is the simplest way to create a Balloon Toolbar. It uses {@link CKEDITOR.plugins.balloontoolbar.contextManager#create} underneath.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line could be simplified something like:

  • Use {@link CKEDITOR.plugins.balloontoolbar.contextManager#create} method to register a new toolbar context.

@@ -376,6 +376,21 @@
}

ContextManager.prototype = {

/**
* Creates {@link CKEDITOR.plugins.balloontoolbar.context} from the given options.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should repeat returned type explicitly here. It's enough it's in the @returns tag.

Let's just make it something like "Creates a toolbar context", so that we reduce change that future maintainer will change return value in one place, and not the other.

It would be also nice to add the information that this method (as opposed to add method) creates, adds and returns the context.

I would also love to see a short code listing here (yea I know that one was in the editor.balloonToolbars property, but I think it won't do harm).

@mlewand
Copy link
Contributor

mlewand commented Nov 27, 2017

Just a couple of minor doc details. Proposed my changes in #1245.

@f1ames
Copy link
Contributor Author

f1ames commented Nov 28, 2017

Merged changes and rebased ont/933, ready for R.

@f1ames f1ames requested a review from mlewand November 28, 2017 09:17
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

@mlewand mlewand merged commit f8d88ee into t/933 Nov 28, 2017
@mlewand mlewand deleted the t/933-1237 branch November 28, 2017 09:21
mlewand added a commit that referenced this pull request Nov 29, 2017
Move creation API to the editor directly
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