liferay / liferay-ckeditor

Other
8 stars 49 forks source link

fix: modify calculation for dialog defaultWidth which caused visual bug #133

Closed mateomustapic closed 4 years ago

mateomustapic commented 4 years ago

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 beforechrome

Before Firefox beforefirefox

After Chrome

afterchrome

After Firefox

afterfirefox
julien commented 4 years ago

gonna test this right now :partying_face:

julien commented 4 years ago

@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:

firefox02

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 commented 4 years ago

@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 commented 4 years ago

@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

ie_before

After

ie_witdth_test
julien commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I need to wear glasses

jbalsas commented 4 years ago

Why don't simply use foo / 2?

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

julien commented 4 years ago

@jbalsas that's also true - even more obvious

mateomustapic commented 4 years ago

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

julien commented 4 years ago

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