Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Revert view element's original width after resize #114

Closed
wants to merge 2 commits into from

Conversation

phaux
Copy link

@phaux phaux commented Jan 10, 2020

Fix: Revert view element's original width after resize and let the model handle it

Closes ckeditor/ckeditor5#6060

@phaux phaux requested a review from scofalik January 10, 2020 13:51
@@ -188,8 +188,16 @@ export default class Resizer {
const unit = this._options.unit;
const newValue = ( unit === '%' ? this.state.proposedWidthPercents : this.state.proposedWidth ) + this._options.unit;

this._options.editor.editing.view.change( writer => {
writer.setStyle( 'width', this.state.originalWidth + 'px', this._options.viewElement );
Copy link
Contributor

@scofalik scofalik Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this.state.originalWidth, well, the original width of the image? Or the naming here is ... unfortunate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it looks like it is the width at the start of the resize process 🤦‍♂ . Please confirm that this value is what we wanted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with originalWidth is that it is a calculated original width (as we need real pixels count for some math later on).

You could apply it back to the view before committing the results, but that would just cover up the problem - as it would still not necessarily be the original value (e.g. when initially image view has no width, or width in units like em, pt - which gets converted).

The proper solution would be to store view's initial width and reapply it just before calling onCommit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick and dirty PoC pushed to i/6060a branch, see if that's what you're looking for.

this._options.onCommit( newValue );

const domHandleHost = this._getHandleHost();
const domHandleHostRect = new Rect( domHandleHost );
this.redraw( domHandleHostRect );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this is needed and what happens there? AFAICS, the width is set there again. However, I assume that domHandleHostRect sizes are equal to the state.originalWidth and state.originalHeight?

@scofalik
Copy link
Contributor

  1. The naming convention for branches in CKE5 repos is i/<issue_number>.
  2. No tests. However, we can wait for that until we agreed on the final solution.

Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R-.

@Reinmar
Copy link
Member

Reinmar commented Jan 15, 2020

@mlewand could you also look into this PR so we can verify the proposed changes before they get polished?

Also, we should perhaps wait for #115 to be merged or merge this PR into #115 in which case you'd need to take it from here. WDYT, @mlewand?

@mlewand
Copy link
Contributor

mlewand commented Jan 15, 2020

Also, we should perhaps wait for #115 to be merged or merge this PR

There is high chance that #115 will cause conflicts in the unit tests. Which are not a part of this PR yet, but for sure we'll be adding them.

Asked for some clarifications in ckeditor/ckeditor5#6060 first.

@mlewand
Copy link
Contributor

mlewand commented Apr 17, 2020

Closing this PR in favor of #122. This PR only handles commit (graceful image resize) and doesn't handle resize cancel, which could happen during the process.

@mlewand mlewand closed this Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image resizer should clean the view after itself
4 participants