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

Introduced UpcastConversionApi#getSplitParts #1704

Merged
merged 10 commits into from Mar 26, 2019
Merged

Introduced UpcastConversionApi#getSplitParts #1704

merged 10 commits into from Mar 26, 2019

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Mar 25, 2019

Suggested merge commit message (convention)

Feature: Introduced UpcastConversionApi#getSplitParts. Also, provided a way to set upcast conversion helper fired for every view element. Closes ckeditor/ckeditor5#1580. Closes ckeditor/ckeditor5#1581.


Additional information

I added a possibility to specify conversion from any view element to model element by not setting view property in upcast helper configuration:

conversion.for( 'upcast' ).elementToElement( { model: 'paragraph' } );

Will convert any non-converted element to a paragraph model element.

@scofalik scofalik requested a review from pjasiun March 25, 2019 12:03
@coveralls
Copy link

coveralls commented Mar 25, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 9dc2c6a on t/1580-2 into 42cb5b4 on master.

@scofalik
Copy link
Contributor Author

I've also added integration tests for the issue: ckeditor/ckeditor5-block-quote#34. I wasn't sure where really to put them though :S. I hope this place is fine with you.

* @param {module:engine/model/element~Element} original The original element.
* @param {module:engine/model/element~Element} split The split part element.
*/
_registerSplitPair( original, split ) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
_registerSplitPair( original, split ) {
_registerSplitPair( original, copy ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a huge fan of split name but I think it is better than copy. This is not a copy of that element, they are both split parts. If not split, I'd go with originalPart + newPart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or originalPart and splitPart.

*
* @private
* @param {module:engine/model/element~Element} original The original element.
* @param {module:engine/model/element~Element} split The split part element.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* @param {module:engine/model/element~Element} split The split part element.
* @param {module:engine/model/element~Element} copy The copy of the original element which was created as the result of split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

} else {
// There should not be any text nodes after the element is split, so the only other value is `elementStart`.
const originalElement = stack.pop();
const splitElement = treeWalkerValue.item;
Copy link

Choose a reason for hiding this comment

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

I am not sure about the make splitElement. I think copyElement is much better.

Copy link

Choose a reason for hiding this comment

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

partElement, partialElement or splitPartElement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above, I can agree for originalPart and splitPart or newPart.

parts = this._splitParts.get( element );
}

parts.last = parts[ parts.length - 1 ];
Copy link

Choose a reason for hiding this comment

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

Actually, I am not sure if we should do it. We do not do it for any other array, even considering that in many cases it could be useful. If we will do it now, we may do it more often and end up in the situation where half of arrays has property last and the other half do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was your idea :P But, yeah, I am not sure about it either.

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!Array.prototype.last) { Object.defineProperty... 😈 😄

Copy link

Choose a reason for hiding this comment

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

:trollface:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants