Feature: Initial implementation of ButtonDropdown View. #339
Conversation
# Conflicts: # theme/theme.scss
src/buttongroup/buttongroupview.js
Outdated
| * | ||
| * @extends module:ui/view~View | ||
| */ | ||
| export default class ButtonGroupView extends View { |
There was a problem hiding this comment.
I wonder if this class could be replaced by the ToolbarView? It almost replicates its functionality 1:1. The only feature that ToolbarView would need is isVertical, right?
There was a problem hiding this comment.
Looks like it is a 1:1 copy with additional isVertical property behavior. It would only need some minor CSS adjustments for paddings:
The other things is (probably minor) the name of this thing as I don't feel too comfortable with placing Toolbar inside dropdown. But as it works and does not create bloat code I can live with this :)
| * | ||
| * const buttons = []; | ||
| * | ||
| * buttons.push( new ButtonView() ); |
| return dropdownView; | ||
| } | ||
|
|
||
| function getBindingTargets( buttons, attribute ) { |
src/dropdown/utils.js
Outdated
|
|
||
| /* global document */ | ||
|
|
||
| export function openDropdownOnArrows( dropdownView, buttonGroupView ) { |
src/dropdown/utils.js
Outdated
| } ); | ||
| } | ||
|
|
||
| export function closeDropdownOnExecute( dropdownView, items ) { |
theme/components/buttongroup.css
Outdated
| @@ -0,0 +1,11 @@ | |||
| // Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | |||
There was a problem hiding this comment.
This file, if ButtonGroupView becomes a component (my comment in the class definition), should have its own folder.
|
|
||
| dropdownView.buttonView.extendTemplate( { | ||
| attributes: { | ||
| class: [ 'ck-button-dropdown' ] |
There was a problem hiding this comment.
I think classes are set on wrong DOM elements. ATM it's
<div class="ck-dropdown">
<button class="ck-button ck-enabled ck-off ck-dropdown__button ck-button-dropdown" type="button" tabindex="-1">...</button>
<div class="ck-reset ck-dropdown__panel">
<div class="ck-reset ck-button-group ">...</div>
</div>
</div>but I think it should be
<div class="ck-dropdown ck-button-dropdown">
<button class="ck-button ck-enabled ck-off ck-dropdown__button" type="button" tabindex="-1">...</button>
<div class="ck-reset ck-dropdown__panel">
<div class="ck-reset ck-button-group ">...</div>
</div>
</div>instead. WDYT?
src/buttongroup/buttongroupview.js
Outdated
| /** | ||
| * @inheritDoc | ||
| */ | ||
| constructor( options = {} ) { |
There was a problem hiding this comment.
Docs for options? View does not accept such argument, so @inheritDoc won't work.
src/buttongroup/buttongroupview.js
Outdated
| * @inheritDoc | ||
| */ | ||
| constructor( options = {} ) { | ||
| super(); |
There was a problem hiding this comment.
I think it's safe to pass locale to the parent here. If it's needed in the future, it will require changes in many places that use this component because the order of constructor() arguments will change (locale should be first).
src/buttongroup/buttongroupview.js
Outdated
| import '../../theme/components/buttongroup.css'; | ||
|
|
||
| /** | ||
| * The button group view class. |
|
@oleq I've updated this PR with:
|
….ck-disabled class to the attribute.
…ument from createButtonDropdown helper.
…View#className inside the dropdown.
…horizontal space.
oleq
changed the title
…nModel#isOn instead of manipulating the CSS in the dropdown.
Suggested merge commit message (convention)
Feature: Initial implementation of ButtonGroupView and ButtonDropdown View. Closes ckeditor/ckeditor5#5428.
Additional information
This PR mostly required by alignment feature but proposed button dropdown is a generic component.
I've decided to use Buttons directly to enable easy button wrapping to button dropdown.
This PR also requires change in theme lark: https://github.com/ckeditor/ckeditor5-theme-lark/tree/t/ckeditor5-ui/333 with proposed merge commit msg: