t/103: Update components once UI architecture has been stripped of Controller class #104
Conversation
| @@ -0,0 +1,63 @@ | |||
| /** | |||
There was a problem hiding this comment.
createDropdown - camelCase in file name.
| * The label of the dropdown. | ||
| * | ||
| * @observable | ||
| * @member {String} ui.dropdown.DropdownView#label |
There was a problem hiding this comment.
I think we should move docs of class properties to the class constructor.
| * | ||
| * @observable | ||
| * @member {Boolean} ui.dropdown.DropdownModel#withText | ||
| */ |
There was a problem hiding this comment.
I can't find docs for ui.dropdown.DropdownModel#items
There was a problem hiding this comment.
Ok, now I see that there is a createListDropdown helper
There was a problem hiding this comment.
Yep. We'll need to figure out the right way to document stuff here. But ATM I see no better solution than Model @interface.
| } | ||
|
|
||
| return label; | ||
| } ); |
There was a problem hiding this comment.
I remember we agreed that the title should be built in the ButtonView. It should have title property, which, if set, is used. Otherwise, a label with a keystroke (if exists) is used.
There was a problem hiding this comment.
Then, the whole createButton helper is not needed anymore. In fact, we should avoid the need to create a createButton helper and try to keep the whole logic inside the view class.
…ctoring in both classes.
…ton() helper function.
… binding helper tests.
| @@ -64,64 +146,62 @@ export default class ButtonView extends View { | |||
| // We can't make the button disabled using the disabled attribute, because it won't be focusable. | |||
| // Though, shouldn't this condition be moved to the button controller? | |||
| if ( this.isEnabled ) { | |||
| this.fire( 'click' ); | |||
| this.fire( 'execute' ); | |||
There was a problem hiding this comment.
Because you can do the same with keyboard Return/Space keys, touch it on your smartphone, etc. click events are limited to the mouse and developers who use ButtonView don't actually need to know what kind of interaction executed the button. execute is an interface which is uniform and brings no confusion.
| @@ -18,8 +21,87 @@ export default class ButtonView extends View { | |||
| /** | |||
| * @inheritDoc | |||
| */ | |||
| constructor() { | |||
| super(); | |||
| constructor( locale ) { | |||
There was a problem hiding this comment.
The second parameter with options (properties) would be nice, so I can create a button with label, instead of creating anonymous button and adding label later.
const buttonView = new ButtonView( locale, {
laber: 'foo',
withText: true
} );
There was a problem hiding this comment.
OTOH, you can always
const buttonView = new ButtonView( locale );
buttonView.set( {
laber: 'foo',
withText: true
} );
There was a problem hiding this comment.
Yea, but it means that you create a view with empty properties and then add properties to it. It means that button is in the invalid state for a moment. If we would add assertions to check if the view is not in the invalid state it would throw in this case. And, even if it is fine for the button to have no label nor image, it might be not fine for other views to have an undefined initial state.
Closes #103.
Requires ckeditor/ckeditor5-ui#100.