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

Update image.js #134

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@eon8
Copy link
Contributor

eon8 commented Oct 6, 2014

We have to update preview after the original image was loaded.
If we do that - the preview will have correct size after dialog is opened.
At this moment an image with a reduced size has 100% dimensions in dialog preview block before we make any changes and another update is triggered.

Update image.js
We have to update preview after the original image was loaded.
If we do that - the preview will have correct size after dialog is opened.
At this moment an image with a reduced size has 100% dimensions in dialog preview block before we make any changes and another update is triggered.
@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Oct 6, 2014

Hey. Thanks for the patch. Could you, however, explain step by step what we have to do to reproduce the issue?

@eon8

This comment has been minimized.

Copy link
Contributor Author

eon8 commented Oct 6, 2014

Yes, of course. Steps to reproduce:

  1. Open editor and add an image.
  2. Go to it's properties and make the size smaller than original.
  3. Apply changes and close the dialog.
  4. Open that dialog again.

Actual result: the image in preview still has it's original size.
Expected: preview image should have width and height from settings.

On Mon, Oct 6, 2014 at 6:01 PM, Piotrek Reinmar Koszuliński <
notifications@github.com> wrote:

Hey. Thanks for the patch. Could you, however, explain step by step what
we have to do to reproduce the issue?


Reply to this email directly or view it on GitHub
#134 (comment).

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Oct 9, 2014

I can't reproduce this issue. I checked it on Chrome, Firefox and IE11. The preview is always loaded with the size set in the dialog. I tried local files and some external ones, to check for timing problems, but it's still ok.

Can you reproduce the issue on http://ckeditor.com/demo? Or does it require some additional configuration?

@eon8

This comment has been minimized.

Copy link
Contributor Author

eon8 commented Oct 16, 2014

Sorry for delay. Check this video
https://docs.google.com/file/d/0B7XlhMgC0zCOdnAxYXp5UEhvdjQ/edit?usp=drivesdk,
recorded on ckeditor demo webpage. (Chrome 38.0.2125.101)

On Thu, Oct 9, 2014 at 11:42 AM, Piotrek Reinmar Koszuliński <
notifications@github.com> wrote:

I can't reproduce this issue. I checked it on Chrome, Firefox and IE11.
The preview is always loaded with the size set in the dialog. I tried local
files and some external ones, to check for timing problems, but it's still
ok.

Can you reproduce the issue on http://ckeditor.com/demo? Or does it
require some additional configuration?


Reply to this email directly or view it on GitHub
#134 (comment).

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Dec 1, 2014

By a coincidence we found a ticket which might be related to this issue: https://dev.ckeditor.com/ticket/9759.

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Jan 14, 2015

This is a weeeeird issue. I spent a good time trying to figure out WTH and didn't succeed. I can reproduce it but only on editor loaded from CDN (our demo uses our CDN) what made it impossible to debug.

Your fix looks good though, so I decided to give it a try. However, I'm not able to verify that it indeed fixes the issue, so I closed it conditionally. We'll wait until the release and if the next version (4.4.7) turns out to be ok, we'll ultimately close this issue. This is a ridiculous way of closing a ticket, because we could try to test a RC on the CDN, but I don't want to spend more time on so small detail :D.

I reported a ticket: http://dev.ckeditor.com/ticket/12818 and merged your change to master with 89e2c9b. We'll add a changelog entry when we'll verify that everything works.

@Reinmar

This comment has been minimized.

Copy link
Member

Reinmar commented Jul 31, 2015

OK, I can confirm it's fixed now. I could not be able to reproduce the same issue on our demo. Thanks again for the patch. We can now close the issue :)

@Reinmar Reinmar closed this Jul 31, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.