Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing DataProcessor#registerRawContentElementMatcher() #8529

Merged
merged 9 commits into from
Nov 28, 2020
Merged

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Nov 25, 2020

Suggested merge commit message (convention)

Feature (engine): Introducing DataProcessor#registerRawContentMatcher()

Fix (html-embed): Editor should not crash after inserting a broken HTML. Closes #8323.

Fix (engine): DomConverter should not trim whitespaces in nodes that are siblings to inline raw content elements (e.g. MathML). Closes #5870.


Additional information

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

I would add more tests for the new method. We should add more test cases to define what happens in a scenario, where two or more features registers raw content.

The method name is a bit long, so I've proposed a shorter one as I think that Element part is redundant. But that's up to you to decide whether you think that's a valid comment.

@@ -158,6 +158,42 @@ describe( 'DomConverter', () => {
expect( viewFragment2 ).to.equal( viewFragment );
} );

it( 'should assign $rawContent custom property for view elements registered as raw content elements', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Too few tests IMO. Right now it tests only one pattern. I think that we could check a bit more:

  • multiple patterns registered
  • what if div.raw-content-container has other attributes, classed or styles, ie div[ data-foo="bar"].raw-content-container

Co-authored-by: Maciej <jodator@jodator.net>
niegowski and others added 3 commits November 26, 2020 15:21
Co-authored-by: Piotrek Koszuliński <pkoszulinski@gmail.com>
@niegowski niegowski requested a review from jodator November 27, 2020 11:10
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@jodator jodator merged commit b8538de into master Nov 28, 2020
@jodator jodator deleted the i/8323 branch November 28, 2020 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants