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

Split ImageResize to ImageResizeEditing and ImageResizeUI #5188

Closed
mlewand opened this issue Aug 19, 2019 · 2 comments
Closed

Split ImageResize to ImageResizeEditing and ImageResizeUI #5188

mlewand opened this issue Aug 19, 2019 · 2 comments
Assignees
Labels
package:image resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@mlewand
Copy link
Contributor

mlewand commented Aug 19, 2019

Is this a bug report or feature request?

Task

📋 Steps to reproduce

Currently there's only a ImageResize plugin. It should be split as other plugins, into Editing and UI.

@jodator
Copy link
Contributor

jodator commented Aug 21, 2019

To not create too many issues: The code coverage for imageresize.js is below 100% - a missing test case for overwriting converter.

And since I was looking at the test few things that I don't like there:

  1. The docs for generateResizeTest() could be updated and moved from funciton body to normal // hidden jsdoc format, ie:
    // Desc...
    //
    // @param {Object} options
    // @param {String} options.resizerPosition
    funciton generateResizerTest( options )
  2. I like the idea of function that generates tests that returns a function rather then creating internal it() call 👍
  3. I'm not sure if I like the optionsmodelRegExp - it' too complex to read in a test case I think that would be better to rewrite this, ie:
    • the <paragraph>foo</paragraph> bloats test - I'd remove it completly or make it transparent to the tests methods
    • the amount of width=([\d]{2}(?:\.[\d]{1,2})) is also too much - couldnt we create some expectansion for this?
      generateResizeTest( {
          // .. other options
          expect: {
              width: `45.6%` // or even a regex for value if some computational quirks
          }
      } )
    • The options.expectedWidth might be options.expect.domWidth (the DOM part is missing in the variable) I've get to the methods internals - I don't see point in testing model RegExp when we can simply check model values and only regexp test them instead of a whole model contents. It is really hard to read :(

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

The entire part of ImageResize belongs to ImageResizeEditing (no other UI parts right now) so for now let's keep a single class.

@Reinmar Reinmar closed this as completed Aug 27, 2019
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-image Oct 9, 2019
@mlewand mlewand added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:image labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants