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

Sharedspace plugin now supports ckeditor elements and not only ids #187

Closed
wants to merge 1 commit into from

Conversation

@Undergrounder
Copy link
Contributor

Undergrounder commented May 19, 2015

The sharedspace plugin should support passing passing dom elements and not only ids.

In angular js it has no sense to use ids in directives. (Reusable components).

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented May 19, 2015

Hi. Thanks for the patch. Great to see that it already contains a test :).

Four small things before we can merge it:

  • I think that the setting should accept native DOM elements rather than CKEditor ones. The reason is that later on the config object is cloned and cloning non-native types works poorly.
  • Extend documentation of this option (native element's type is {HTMLElement}).
  • Please remove console logs, comment in L132 and code style in both JS files. You can read here about contributing patches where code style checker and linter are also mentioned.
  • You can remove L55-L57 in tests.
@Undergrounder

This comment has been minimized.

Copy link
Contributor Author

Undergrounder commented May 20, 2015

All changed and tested.

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented May 20, 2015

Great! Your PR was merged to major with 1d283a5. Thanks again for the work.

@Reinmar Reinmar closed this May 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.