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

Image resizer should clean the view after itself #6060

Closed
scofalik opened this issue Jan 9, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-widget#122
Closed

Image resizer should clean the view after itself #6060

scofalik opened this issue Jan 9, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-widget#122
Assignees
Labels
package:image type:bug This issue reports a buggy (incorrect) behavior.

Comments

@scofalik
Copy link
Contributor

scofalik commented Jan 9, 2020

📝 Provide detailed reproduction steps (if any)

At the moment, Resizer makes changes directly in the view. This is not recommended usually but in this case, it makes sense (we don't want to so many changes done on model).

What I don't like is that the Resizer does not clean after itself. When the drag ends and the commit method is fired and onCommit callback is executed, it should clean the changes (re-set width to the value it was before resizing).

@scofalik scofalik added type:bug This issue reports a buggy (incorrect) behavior. package:image labels Jan 9, 2020
@Mgsy Mgsy added this to the iteration 29 milestone Jan 14, 2020
@mlewand
Copy link
Contributor

mlewand commented Jan 15, 2020

@scofalik Just to clarify, why would you expect the resizer to clean the DOM after a successful commit? The purpose of the resizer is to resize stuff, so if the action was successful it make sense to reflect this change in the view (as otherwise it would not be visible to the end user).

It's possible that I'm missing something here, like that what you mean here is that the resize command was disabled in meanwhile (in which case yes, it should revert the original image size).

@scofalik
Copy link
Contributor Author

scofalik commented Jan 16, 2020

Not DOM but view and not after a successful commit but at the "drag end" moment.

Right now, you are changing values in the view without basing on the model. Most things in the view (barring some very rare UI stuff) should be an effect of a model change. I'd like the view state to be based on the model.

Now, I understand why we are not changing the width in the model all the time, no worries.

My point of view is that you are creating a UI and then leave it uncleaned. Imagine that instead of resizing the original image, we have a half-transparent "ghost" image over it and you resize it. In this case, you wouldn't have any doubt that you need to remove the "ghost" image from the view, because it was an artifact of the UI interaction. You could say that "image being resized" is kind of a UI.

This is why I'd like to clean the view after the drag ends (commit call) but before .onCommit is called.

The problem isn't that big, though. For two reasons. First: you probably won't have a case where you change width in the view and want that to be converted to a different change. Second: we will be fine with image not returning to its original size in the go-to solution (but not in the "first" solution).

Right now, our case is that in track changes mode, after you end dragging, commit is called, width is set in the view, but the command call (in onCommit) is not fired (instead, we create a suggestion). This results in mismatched model and view. When we discard the suggestion, we still end up with mismatched model and view.

In the future, for image resizing, we'd like the image to stay resized in track changed mode. But this requires more logic on our side with suggestions accepting/rejecting. I wanted to do this in small steps (simple solution now, correct later).

So, as much as we don't care how it will work in the go-to solution, we care how it works in the first solution. If we will be able to introduce the final solution in this iteration, this discussion will stay academical.

But, it doesn't mean that UI artifacts should not be cleaned after the UI has done its work :).

@mlewand
Copy link
Contributor

mlewand commented Jan 16, 2020

Thanks for shedding more light on this @scofalik - yes in this case it makes a perfect sense not to change the markup (actually the process should be cancelled in a first place, but we didn't handle it due to some simplifications).

@scofalik
Copy link
Contributor Author

Soooo, how do we proceed with this? @mlewand @Reinmar.

@Reinmar Reinmar modified the milestones: iteration 29, iteration 30 Feb 12, 2020
@Reinmar Reinmar modified the milestones: iteration 30, iteration 31 Mar 10, 2020
@mlewand mlewand self-assigned this Apr 17, 2020
Reinmar added a commit to ckeditor/ckeditor5-widget that referenced this issue Apr 22, 2020
Fix: Image resize now cleans up temporary view `width` style changes. Closes ckeditor/ckeditor5#6060.
mlewand pushed a commit that referenced this issue May 1, 2020
Fix: Image resize now cleans up temporary view `width` style changes. Closes #6060.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
4 participants