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

ListItemView should react to isEnabled state changes. #390

Merged
merged 8 commits into from
Jun 5, 2018
Merged

ListItemView should react to isEnabled state changes. #390

merged 8 commits into from
Jun 5, 2018

Conversation

jodator
Copy link
Contributor

@jodator jodator commented May 10, 2018

Suggested merge commit message (convention)

Other: The ListItemView now can be in disabled state by changing isEnabled property. Closes ckeditor/ckeditor5#5453.


Additional information

@jodator jodator requested a review from oleq May 10, 2018 15:11
@coveralls
Copy link

coveralls commented May 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling f7c4177 on t/389 into beb9f96 on master.

@@ -49,7 +49,8 @@ export default class ListItemView extends View {
'ck',
'ck-list__item',
bind.to( 'class' ),
bind.if( 'isActive', 'ck-list__item_active' )
bind.if( 'isActive', 'ck-list__item_active' ),
bind.to( 'isEnabled', value => value ? 'ck-enabled' : 'ck-disabled' )
Copy link
Member

Choose a reason for hiding this comment

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

AFAIR the .ck-enabled class is a dummy class that does nothing. Could you check that and get rid of it across the project if you agree?

Searching 3161 files for "ck-enabled" (case sensitive)

/Users/oleq/CK/5/ckeditor5/docs/framework/guides/architecture/ui-library.md:
   38  					// The value of view#isEnabled will control the presence
   39  					// of the class.
   40: 					bind.if( 'isEnabled', 'ck-enabled' ),
   41  				],
   42  
   ..
  111  
  112  ```html
  113: <input class="foo ck-enabled" type="text" placeholder="Type some text" />
  114  ```
  115  

/Users/oleq/CK/5/ckeditor5/packages/ckeditor5-ui/src/button/buttonview.js:
  119  					'ck',
  120  					'ck-button',
  121: 					bind.to( 'isEnabled', value => value ? 'ck-enabled' : 'ck-disabled' ),
  122  					bind.if( 'isVisible', 'ck-hidden', value => !value ),
  123  					bind.to( 'isOn', value => value ? 'ck-on' : 'ck-off' ),

/Users/oleq/CK/5/ckeditor5/packages/ckeditor5-ui/tests/button/buttonview.js:
   51  				expect( view.element.classList.contains( 'ck' ) ).to.true;
   52  				expect( view.element.classList.contains( 'ck-button' ) ).to.true;
   53: 				expect( view.element.classList.contains( 'ck-enabled' ) ).to.true;
   54  				expect( view.element.classList.contains( 'ck-off' ) ).to.true;
   55  			} );

4 matches across 3 files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it is a case. I'll remove it.

* @default true
* @member {Boolean} #isEnabled
*/
this.set( 'isEnabled', true );
Copy link
Member

Choose a reason for hiding this comment

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

I think that, just like the ButtonView, a disabled ListItemView should not execute if disabled.

@oleq oleq merged commit 76a4d47 into master Jun 5, 2018
@oleq oleq deleted the t/389 branch June 5, 2018 13:30
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.

ListItem doesn't support isEnabled state.
3 participants