fix: modify calculation for dialog defaultWidth which caused visual bug

[mateomustapic]

This PR fixes an issue described in https://issues.liferay.com/browse/LPS-120758 ,visual bug in Chrome (scrollbar) and in Firefox (whitespace) in Source dialog box.
This issue was caused by CodeMirror plugin line number width not included in dialog size calculation.

To test this fix:

1. Build CKEditor with this change
2. Access to portal.
3. Go to Web Content.
4. Add a content.
5. In content field, click Source button in ckeditor toolbar to go to source view.
6. Type any text
7. Click the Expand icon next to the Source button to preview source dialog.

Before Chrome

Before Firefox

After Chrome

After Firefox

[carloslancha]

Just left a nit. @julien will know better the impact of this change :)

[carloslancha]

Is this padding var being used elsewhere? If not I'd avoid creating the variable

[julien]

you're right, it's not being used anywhere, but I'm guessing @mateomustapic added this to avoid a "magic number"

[carloslancha]

Is not 0.5 already a magic number? 😛

[wincent]

Well, it translates pretty obviously to "half" so not all that magical, unlike padding which is totally arbitrary. You probably wouldn't want to write:

var padding = 40;

var half = 0.5;

var defaultWidth = size.width * half - padding;

[julien]

Is not 0.5 already a magic number? stuck_out_tongue

yeah, well you make a point; but I always read that as "whatever / 2"

x / 2 == x * 0.5

[julien]

Sorry I hadn't seen @wincent 's comment before adding mine: now that I've read it, I agree about avoiding those magic numbers everywhere

[julien]

gonna test this right now 🥳

[julien]

@mateomustapic so I tested this and here's what I think (with screenshots):

The bug seems to be gone in Chrome:

and also seems fixed when you have the devtools open or if the window has a smaller size:

Same thing, in Firefox:

And in Firefox with the devtools open:

But while we're looking at it, I also would consider fixing this, because it seems related (I'm not saying it's mandatory but if it's not too hard to fix, why not)

[julien]

@mateomustapic concerning the editor's height in the dialog, I'm going to to that in #132
Just make sure this works fine in IE11 as well please. Thanks

[mateomustapic]

@mateomustapic concerning the editor's height in the dialog, I'm going to to that in #132
Just make sure this works fine in IE11 as well please. Thanks

hey @julien

Just tested this in IE, and works as in other browsers.
Before

After

[julien]

Ok thanks for confirming @mateomustapic, I'm still going to give it a quick test run in DXP because who knows.

Could you tell me what you think of @wincent's comment?

I personally think it's better to avoid those magic numbers, so I'd be OK to do it both for half (0.5) and padding (40)

@carloslancha does that seem reasonable to you as well?

[mateomustapic]

Ok thanks for confirming @mateomustapic, I'm still going to give it a quick test run in DXP because who knows.

Could you tell me what you think of @wincent's comment?

I personally think it's better to avoid those magic numbers, so I'd be OK to do it both for half (0.5) and padding (40)

@carloslancha does that seem reasonable to you as well?

@julien As I can see, @wincent says "_Well, it translates pretty obviously to "half" so not all that magical, unlike padding which is totally arbitrary. You probably wouldn't want to write .... var defaultWidth = size.width * half - padding;" , meaning adding of padding variable is ok, but not adding of half variable, where 0.5 value is self-explanatory.

[julien]

I need to wear glasses

[jbalsas]

Why don't simply use foo / 2?

That's more obviously a "halving" operation than foo * .5 :)

[julien]

@jbalsas that's also true - even more obvious

[mateomustapic]

yeah, it's more readable to divide by 2 than multiply 0.5.

[julien]

The change looks good to me - so I'm merging this one