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

Fixed: List postfixers should check if attribute is allowed od elemen… #104

Merged
merged 3 commits into from
Jun 13, 2018

Conversation

jodator
Copy link
Contributor

@jodator jodator commented May 14, 2018

Suggested merge commit message (convention)

Other: Rename list attributes indent and type to listIndent and listType. Closes ckeditor/ckeditor5#3003.

BREAKING CHANGE: The indent attribute is now called listIndent. See ckeditor/ckeditor5#3003 for more information.
BREAKING CHANGE: The type attribute is now called listType. See ckeditor/ckeditor5#3003 for more information.


Additional information

@jodator jodator requested a review from Reinmar May 14, 2018 15:19
@coveralls
Copy link

coveralls commented May 14, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 9ae6506 on t/103 into 9da2f34 on master.

@@ -602,13 +602,15 @@ export function modelChangePostFixer( model, writer ) {
// In case of renamed element.
const item = entry.position.nodeAfter;

if ( item.hasAttribute( 'indent' ) ) {
// Only remove 'indent' attribute from items that doesn't allow such attribute. #103
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment that this code should be removed once https://github.com/ckeditor/ckeditor5-engine/issues/1228 is resolved?

Copy link
Contributor Author

@jodator jodator May 14, 2018

Choose a reason for hiding this comment

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

The whole check, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Reinmar
Copy link
Member

Reinmar commented Jun 4, 2018

As discussed in https://github.com/ckeditor/ckeditor5-list/issues/103#issuecomment-389079136 we should rather change the name of list item's attributes. Those attrs which are only used with list items should be prefixed.

@jodator
Copy link
Contributor Author

jodator commented Jun 5, 2018

@Reinmar OK I've changed the PR to renaming type to listType and indent to listIndent.

@Reinmar
Copy link
Member

Reinmar commented Jun 12, 2018

What about other repos?

@jodator
Copy link
Contributor Author

jodator commented Jun 13, 2018

What about other repos?

Do you mean like docs in model/schema which uses lists as examples or some other things? I'll update the docs but for the latter you have give me some more hints ;)

ps.: OK I've found some tests in other repos to update also

@Reinmar
Copy link
Member

Reinmar commented Jun 13, 2018

ps.: OK I've found some tests in other repos to update also

Yup, I meant the tests.

@jodator
Copy link
Contributor Author

jodator commented Jun 13, 2018

@Reinmar so it's done - see updated PR comment with proper branches.

@Reinmar Reinmar merged commit 7a1ece6 into master Jun 13, 2018
@Reinmar Reinmar deleted the t/103 branch June 13, 2018 16:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants