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

Fixed Schema#allow() entry. #98

Merged
merged 3 commits into from
Apr 18, 2017
Merged

Fixed Schema#allow() entry. #98

merged 3 commits into from
Apr 18, 2017

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Apr 10, 2017

Suggested merge commit message (convention)

Fix: Schema is now correctly set for Link feature. Closes ckeditor/ckeditor5#4780.

@Reinmar
Copy link
Member

Reinmar commented Apr 11, 2017

Misses a test for the thing which was actually supposed to be fixed.

@Reinmar
Copy link
Member

Reinmar commented Apr 11, 2017

Two tests could be written – what happens with links which are being inserted directly into the root and a simple schema check whether link is allowed directly in a root.

@scofalik
Copy link
Contributor Author

scofalik commented Apr 14, 2017

Misses a test for the thing which was actually supposed to be fixed.

What was actually supposed to be fixed is that <a> wasn't autoparagraphed, but this is an integration test which will fail anyway because of another issue (in paragraph).

We expect <paragraph><$text linkHref="url">foo</$text></paragraph> but after this is fixed we will get <paragraph>foo</paragraph>. Is there any use in writing incorrect test?

Two tests could be written – what happens with links which are being inserted directly into the root

  • If I add it through editor.setData() I won't get any link (ATM).
  • If I will try to add it through model setting util I will get error that schema does not allow text in root
  • If I will add it through doc.batch() it will be inserted and converted but this is adding something against schema rules so that test is incorrect anyway.

and a simple schema check whether link is allowed directly in a root.

I'll add this one but I don't tests like this (as there are infinite examples where text with link is not allowed).

@scofalik
Copy link
Contributor Author

Okay you know what, I'll write that integration test. It will fail after the issue in paragraph is fixed and then we will fix that test so we won't forget about that case.

@Reinmar
Copy link
Member

Reinmar commented Apr 18, 2017

We expect <$text linkHref="url">foo</$text> but after this is fixed we will get foo. Is there any use in writing incorrect test?

IMO, there's a lot of sense in writing such tests. We will have to change that incorrect test once paragraph is fixed. You will know very well that something has changed because it will start failing. For now, the test will just confirm how it currently behaves and that there are no other issues (e.g. it does not crash).

@Reinmar
Copy link
Member

Reinmar commented Apr 18, 2017

Okay you know what, I'll write that integration test. It will fail after the issue in paragraph is fixed and then we will fix that test so we won't forget about that case.

:D I've read it after writing my previous comment. So, yep – that's the idea.

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

Successfully merging this pull request may close these issues.

Schema is set incorrectly.
2 participants