Conversation
Comandeer
left a comment
There was a problem hiding this comment.
Looks good at the first sight! However, I'm wondering if the introduction of the temporary variable is really needed. I was able to replace it with a direct reference to list item styles and it still worked in the manual test. Could you check it?
| } | ||
| } | ||
|
|
||
| delete element.attributes[ 'cke-list-style-margins' ]; |
There was a problem hiding this comment.
Probably it could be moved to the CKEDITOR.plugins.pastefromword.lists.cleanup function, alongside removal of other temporary attributes.
| * @member CKEDITOR.plugins.pastetools.filters.word.lists | ||
| */ | ||
| createLists: function( root ) { | ||
| createLists: function( root, editor ) { |
There was a problem hiding this comment.
Please update API docs for this method. The same goes for the dissolveList() one.
|
|
||
| var value = CKEDITOR.tools.convertToPx( parsedStyles[ key ] ); | ||
| if ( value === 0 ) { | ||
| zeroMargins += key + ': ' + value + 'px; '; |
There was a problem hiding this comment.
Nitpick: in case of zero value, the unit can be skipped:
| zeroMargins += key + ': ' + value + 'px; '; | |
| zeroMargins += key + ': ' + value; |
|
|
||
| // Preserve keeping list margins zero when pasteTools_keepZeroMargins is ON. #5316 | ||
| if ( keepZeroMargins ) { | ||
| innermostContainer.attributes.style = firstLevel1Element.attributes[ 'cke-list-style-margins' ]; |
There was a problem hiding this comment.
Is that temporary attribute really needed? From my quick test, it seems that firstLevel1Element styles contain margin styles at this point and can be used directly instead.
There was a problem hiding this comment.
I've checked it in my first approach but it doesn't work for me. After your review, I've checked it again and it works well. 🤔 Moreover, the changes are much simpler than adding a temporary variable
|
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
|
Thanks, @Comandeer for the review. |
Comandeer
left a comment
There was a problem hiding this comment.
Zero margins seem to work correctly but there is some issue with non-zero ones. I've applied the margin to the list and it's rendered in the editor as the margin between each of the list items when in Word is rendered only before and after the list (sample document). Could you check if we can fix it in some easy way?
Additionally, it would be good to add some tests for non-zero margins.
|
Thanks @Comandeer for the review. I've added support for non-zero margins for list by default when ACF will be properly set. In addition I've added missing unit and manual test for non-zero ones and PR is ready for another check. |
Comandeer
left a comment
There was a problem hiding this comment.
The logic looks good and margins are preserved. But there are failing tests due to these changes:
Could you check them? I assume that it would be enough to update fixtures to include the information about list margins.
|
@Comandeer, fixtures updated |
|
Rebased on to the latest |
Comandeer
left a comment
There was a problem hiding this comment.
I'm wondering about changes to horizontal margins (left and right). We already support them via indent plugins and probably shouldn't touch it at all – especially since the current logic incorrectly handles them (see margin-left.docx).
We should probably stick only to the vertical ones (top and bottom).
|
@Comandeer handling of the horizontal margins in lists images was removed. |
|
Rebased onto the latest |
Comandeer
left a comment
There was a problem hiding this comment.
Looks good, there is one small improvement left.
| }, 0 ); | ||
| } | ||
|
|
||
| function getMargins( styles ) { |
There was a problem hiding this comment.
Probably we should get margins from both the first and the last list elements – as Word uses the first one to apply margins before the list and the last one to apply the margin after the list – see list-different-margins.docx.
|
|
||
| var value = CKEDITOR.tools.convertToPx( parsedStyles[ key ] ); | ||
|
|
||
| // Preserve keeping zero list margins when pasteTools_keepZeroMargins is ON. #53163 |
There was a problem hiding this comment.
| // Preserve keeping zero list margins when pasteTools_keepZeroMargins is ON. #53163 | |
| // Preserve keeping zero list margins when pasteTools_keepZeroMargins is ON (#53163). |
There was a problem hiding this comment.
Looks like most of the fixtures are the same as on master, only formatting differs. Could you revert them to their master's versions?
|
@Comandeer PR is ready to review. |
Comandeer
left a comment
There was a problem hiding this comment.
LGTM! I've just added a missing semicolon between zero margins in the getMargin() function.
What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
Did you follow the CKEditor 4 code style guide?
Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.
What is the proposed changelog entry for this pull request?
What changes did you make?
As lists are unwrapped during the cleaning process and we lost information about margins, I've added a temporary attribute called
cke-list-style-marginsto save information about zero margins. Then margins are applied during creating a new list when the pasteTools_keepZeroMargins config option is ON.To prepare a unit test I've used our getClipboard tool and I've take an appropriate html from each browser.
Which issues does your PR resolve?
Closes #5316.