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

Added defaultValue to ManualDecorators. #258

Merged
merged 6 commits into from
Apr 7, 2020
Merged

Added defaultValue to ManualDecorators. #258

merged 6 commits into from
Apr 7, 2020

Conversation

niegowski
Copy link
Contributor

Suggested merge commit message (convention)

Feature: Added defaultValue to ManualDecorators. Closes ckeditor/ckeditor5#6031.


Additional information

@coveralls
Copy link

coveralls commented Apr 2, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 5ea7fac on i/6031 into b86aa7f on master.

@@ -324,25 +332,33 @@ describe( 'LinkCommand', () => {

expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]bar' );
} );

it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why storing the false value in the model? Also, how do you actually remove the value from the model?

Copy link
Member

Choose a reason for hiding this comment

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

As I see this – the only thing that needs to change when defalutValue is true is the state of this button in the UI when you create a new link. Since it'll be on at that point, the command will be executed with isSth:true and then our job's done. The rest will work the way it works so far. Do I miss something?

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 discussed on a call:

  • LinkFormView is binding its isOn property to value of manual decorator,
  • value of a manual decorator is updated every time LinkCommand is refreshed,
  • every time value is extracted from the model's attribute,
  • we need to store false in the attribute to indicate that user already decided whether the manual decorator is supposed to be applied or not

@@ -324,25 +332,33 @@ describe( 'LinkCommand', () => {

expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]bar' );
} );

it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why storing the false value in the model? Also, how do you actually remove the value from the model?

Copy link
Member

Choose a reason for hiding this comment

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

I checked that I correctly read the code – it indeed puts the false value in the model.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

Waiting for clarification.

@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2020

Based on manual tests all work fine.

@niegowski niegowski requested a review from Reinmar April 7, 2020 08:31
@Reinmar
Copy link
Member

Reinmar commented Apr 7, 2020

After looking at this PR again, I still have doubts regarding keeping the false value in the model. Isn't it equal in meaning to the lack of the attribute?

The problem is that a decorator which default value is false has two modes in the model:

  • when its attribute is not present – it's off,
  • when its present – it's on.

The decorator which default value is true has 3 modes:

  • when its attribute is present and set to true – it's on,
  • when its attribute is present and set to false – it's off,
  • when its attribute is not present it's a mess – it's off up until opening the link balloon again where it's automatically set to "on" and then saved as such.

Unfortunately, from the model perspective this is such a big inconsistency, that I'd work on resolving it. For instance, a link might've been inserted by a tool which knows nothing about decorators. In this case, it will insert a link with just linkHref and no decorator set to false, which creates the situation I described in the 3rd point.

To sum up – there should always be only 2 modes. In case of decorators which default value is set to true:

  • when its attribute is present and set to true – it's on,
  • when its attribute is not present – it's off and we should assume that it was set to that value.

I know that how the UI is implemented makes this tricky, but there's definitely some option there.

}

// We must check if all decorator values are set on view element.
const decoratorValue = Object.entries( decorator.attributes ).every( ( [ key, value ] ) => {
Copy link
Member

Choose a reason for hiding this comment

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

I wanted to write that this comparison is not precise enough because you can have two decorators using the class attribute but I can see that this is not supported already. It's a pity because the engine treats each class separately (styles and classes are special types of attributes) and this could work. In fact, getData() works, but setData() breaks.

Copy link
Member

Choose a reason for hiding this comment

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

I reported a ticket for that in ckeditor/ckeditor5#6571.

return;
}

// We must check if all decorator values are set on view element.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We must check if all decorator values are set on view element.
// We must check if all decorator values are set on view element.
// The comparison is naive (doesn't handle granular classes or styles)
// but link decorators don't support them anyway (#6571).

@niegowski niegowski requested a review from Reinmar April 7, 2020 18:16
@Reinmar Reinmar merged commit 82f966e into master Apr 7, 2020
@Reinmar Reinmar deleted the i/6031 branch April 7, 2020 19:22
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.

Allow to default a manual link decorators value
3 participants