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

Custom data support for widget commands #145

Closed
wants to merge 4 commits into from
Closed

Conversation

zipp3r
Copy link
Contributor

@zipp3r zipp3r commented Dec 2, 2014

Just pass the second editor.execCommand argument to Widget instance initialization

@Reinmar
Copy link
Member

Reinmar commented Dec 9, 2014

Hey. It's a great idea. Just two things:

  1. It would be safer to pass the widget data as part of another object: editor.execCommand( 'someWidget', { startupData: { ... } } ). This will allow passing more properties in the future without making a non-backward compatible change. E.g. one may want to be able to say if a newly created widget should be focused.
  2. This feature will need at least one test (e.g. right after this one: https://github.com/ckeditor/ckeditor-dev/blob/master/tests/plugins/widget/definition.js#L402). You can read more about CKEditor testing environment here: http://docs.ckeditor.com/#!/guide/dev_tests

@zipp3r
Copy link
Contributor Author

zipp3r commented Dec 23, 2014

@Reinmar, hi, and thanks for the advices!

@teameh
Copy link
Contributor

teameh commented Dec 30, 2014

Nice, mine was almost identical. #152 I looked for PR's but did not find this one apparently. @Reinmar are you asking me or @zipp3r to edit our pull request and implement your suggested way of handeling the data?

Btw I think mine and @zipp3r's solution would not be a complete implementation.

If we want the match the docs and add commandData as a param to the command function:

// You can execute an automatically defined command to
// insert a new simplebox widget or edit the one currently focused.
editor.execCommand( 'simplebox', commandData);

We should als implement the edit function around https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/widget/plugin.js#L1041 to let the function work with focussed widgets.

@Reinmar
Copy link
Member

Reinmar commented Jan 12, 2015

I made a small cleanup (code style, variable name), added API docs (pity that commands are not documented :() and merged changes to the major branch. See 2dbc4f3.

We should also implement the edit function

I think it's ok the way you guys proposed it. It makes sense that this data is passed only to the constructor, especially that the property is called startupData. It does not have to affect the edit() call, because instead of executing the command, one could simply execute the setData() method and, if needed, then the command. Basically, the widget creation scenario is the most complex one there, so it makes sense to cover only it, because in the other scenarios a developer can simply call the right method directly.

Thanks for the great contribution, guys!

@Reinmar Reinmar closed this Jan 12, 2015
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.

3 participants