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

API to get dialog's subject of change / model #2423

Closed
mlewand opened this issue Sep 18, 2018 · 4 comments · Fixed by #2488
Closed

API to get dialog's subject of change / model #2423

mlewand opened this issue Sep 18, 2018 · 4 comments · Fixed by #2488
Labels
changelog:api A changelog entry should be put in the API section of the changelog. plugin:dialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:feature A feature request.
Milestone

Comments

@mlewand
Copy link
Contributor

mlewand commented Sep 18, 2018

Are you reporting a feature request or a bug?

Feature request

Provide detailed reproduction steps (if any)

Currently there's no common way to access a subject of change for a dialog, instead each dialog implements this in a custom way.

Having this unified would allow us to add a method like dialog.isEditing() that tells whether a new entity is currently created or an existing one is created. We often need this kind of information.

Note that since the dialogs are static, isEditing method would need to accept editor instance as an argument.

Proposal

Add a new interface that might be implemented in dialogDefinition, let's call it ModeledDialog.

It features a single method:

  • getModel( editor ) : [Object/null]
    • For the most plugins like table / link it would simply return a CKEDITOR.dom.element if there's a related element to it. For widget plugins (image2, placeholder) it would return widget instance that is a subject of this dialog.
    • Returns null if there's no related model (about, help dialogs).

If the dialog does not implement getModel() it is assumed to be always in editing mode (backward compatible).

Compatibility

Compatibility is a priority here. In case we have some workarounds in published plugins, like:

@mlewand mlewand added type:feature A feature request. changelog:api A changelog entry should be put in the API section of the changelog. labels Sep 18, 2018
@mlewand
Copy link
Contributor Author

mlewand commented Sep 18, 2018

Let's start the discussion first to understand what's the best direction here. I came up with this proposition while reviewing #2277 - as we need an information whether the dialog is editing something or adding.

@jacekbogdanski
Copy link
Member

I'm wondering if we need a function returning model. Could't be just isEditing function returning true as a default to overwrite?

However, we already have a request for putting a widget into dialog events (#1044) so it looks like this feature may not be redundant.

It may play nice with #2277 where the new feature will be enabled if plugin or widget is adapted to it, ie. implements getModel function.

Can we assume that model existence is synonymous with isEditing mode? I don't see a problem with our plugins / widgets, but maybe there are cases where this mode should be set explicitly?

@mlewand
Copy link
Contributor Author

mlewand commented Sep 19, 2018

The function for returning the model is needed anyway so it make sense to add it. What's more is that if you implement getModel you don't have to implement isEditing function anymore (as long as generic implementation covers handling of common types, such as DOM elements, widgets etc.).

@mlewand mlewand added the plugin:dialog The plugin which probably causes the issue. label Sep 26, 2018
@mlewand mlewand added status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. labels Oct 16, 2018
@mlewand mlewand mentioned this issue Oct 16, 2018
2 tasks
@jacekbogdanski
Copy link
Member

I'm wondering if we could also enable definition name with this feature. It seems like the name is hidden under dialog internals, but getModel function gives a lot of options to handle special cases for the given plugins. As an example:

editor.on( 'dialogHide', function( evt ) {
  var dialog = evt.data;
  // Plugin name hidden under private object,
  // we could make it accessible from `dialog.name`
  if ( dialog._.name === 'link' ) {
    var link = dialog.getModel();
    link.data( 'tracking-id', generateTrackingId( link.getAttribute( 'href' ) );
  }
} );

Similar feature request was already asked here: #2669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:api A changelog entry should be put in the API section of the changelog. plugin:dialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:feature A feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants