Tests: New cases which include ranges in blocks in OT tests #1494
Conversation
| expectClients( '<paragraph>BarFoo</paragraph>' ); | ||
| } ); | ||
|
|
||
| it( 'remove attribute from text with 2 attributes in same path', () => { |
There was a problem hiding this comment.
I'd change those examples so that typing is in the remove attribute range.
|
|
||
| it( 'remove attribute from text in different path', () => { | ||
| john.setData( '<paragraph>F[oo]</paragraph><paragraph>Bar</paragraph>' ); | ||
| kate.setData( '<paragraph>Foo</paragraph><paragraph>[Bar]</paragraph>' ); |
There was a problem hiding this comment.
There's no attribute to remove here.
| @@ -376,6 +532,21 @@ describe( 'transform', () => { | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'text into div in different path', () => { | |||
There was a problem hiding this comment.
I don't like this example because it is rather unrealistic. We do not wrap text. And it is bad to have <p><div></div></p> structure, casue <div> should never be in <p>.
Unless it won't have a negative impact on CC, I'd drop this one.
There was a problem hiding this comment.
I agree with you. I added this kind of tests to treat them as theoretical cases where the range is inside an element. I believe it won't have any impact on CC, so if you're up to remove those (there are more in other files), I'm ok with that.
There was a problem hiding this comment.
I removed those cases, but there will be one remaining, which has an impact on CC.
| @@ -407,6 +578,18 @@ describe( 'transform', () => { | |||
| '</blockQuote>' | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'text into div in same path', () => { | |||
| @@ -440,6 +623,21 @@ describe( 'transform', () => { | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'text in different path', () => { | |||
There was a problem hiding this comment.
We will need to work on those names later cause they aren't really helpful (aren't describing exactly what happens). I understand that we don't want to make sentences of description but it would be good to have more meaningful names. BTW. I am not perfect at that either.
|
|
||
| it( 'text in same path', () => { | ||
| john.setData( '<blockQuote><paragraph>[Foo]</paragraph></blockQuote>' ); | ||
| kate.setData( '<blockQuote><paragraph>[Foo]</paragraph></blockQuote>' ); |
There was a problem hiding this comment.
It is a little more correct to have a collapsed selection at the beginning (inside, not before) of unwrapped element. So instead of [Foo] -> []Foo.
There was a problem hiding this comment.
I mean, it doesn't make any difference (at least right now) but I know that in other places I've used the collapsed-selection-notation so it would be good to be consistent.
There was a problem hiding this comment.
Although maybe it even look better when all the unwrapped content is in the selection. But OTOH, it might give someone an idea that you can partially unwrap an element.
| expectClients( '<paragraph>o</paragraph>' ); | ||
| } ); | ||
|
|
||
| it( 'remove attribute from text with 2 attributes in same path', () => { |
There was a problem hiding this comment.
How about having a test that removes part of non-changed and part of the changed text (changed = removed attributes)?
| ); | ||
| } ); | ||
|
|
||
| it( 'remove attribute from text in different path', () => { |
| } ); | ||
|
|
||
| // This should be moved to attribute.js. | ||
| describe( 'by remove attribute', () => { |
There was a problem hiding this comment.
Why you removed those tests?
There was a problem hiding this comment.
I moved all remove attribute tests to the attribute.js file.
There was a problem hiding this comment.
Okay, didn't see that cause attribute.js was not expanded. What about the wrap test?
There was a problem hiding this comment.
Whoops, I see that wrap, unwrap and split don't have this kind of tests. I'll add them.
There was a problem hiding this comment.
I've meant that there is a removed test in move.js: 'intersecting wrap'. I've commented on that below.
There was a problem hiding this comment.
Whoops, I see that wrap, unwrap and split don't have this kind of tests. I'll add them.
Done.
| @@ -644,99 +669,6 @@ describe( 'transform', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| // Should be in attribute.js. | |||
| describe( 'by remove attribute', () => { | |||
There was a problem hiding this comment.
Why you removed those tests?
| @@ -224,21 +224,6 @@ describe( 'transform', () => { | |||
| '</blockQuote>' | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'intersecting wrap', () => { | |||
There was a problem hiding this comment.
Why you removed that test?
There was a problem hiding this comment.
I removed it, because wrap was on text, not element. We decided to remove those cases.
| @@ -594,94 +590,33 @@ describe( 'transform', () => { | |||
|
|
|||
| expectClients( '<paragraph>FooBar</paragraph>' ); | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'by remove attribute', () => { | |||
There was a problem hiding this comment.
Why you removed those tests?
| } ); | ||
| } ); | ||
|
|
||
| describe( 'by remove attribute', () => { |
There was a problem hiding this comment.
Why you removed those tests?
| @@ -15,94 +15,6 @@ describe( 'transform', () => { | |||
| } ); | |||
|
|
|||
| describe( 'rename', () => { | |||
| describe( 'by remove attribute', () => { | |||
There was a problem hiding this comment.
Why you removed those tests?
Suggested merge commit message (convention)
Tests: New cases which include ranges in blocks in OT tests. Closes ckeditor/ckeditor5#4385 .
Additional information
remove attributescenarios to theattribute.jsfile.