Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the toolbar dropdown that groups other buttons #5428

Closed
oleq opened this issue Oct 30, 2017 · 7 comments · Fixed by ckeditor/ckeditor5-ui#339
Closed

Implement the toolbar dropdown that groups other buttons #5428

oleq opened this issue Oct 30, 2017 · 7 comments · Fixed by ckeditor/ckeditor5-ui#339
Assignees
Labels
package:ui type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@oleq
Copy link
Member

oleq commented Oct 30, 2017

The component should look more or less like this:

image

  1. Should it have anything to do with the DropdownView?
  2. What public API should it implement?
  3. How should toolbar config deal with the component? Should we allow the toolbar config to group buttons in such way? I think not but maybe there's some smart way to translate the config to the group. There's the problem with configuring the icon and the behavior of the group button (action on click, state).
  4. The button and the children must be accessible using the keyboard.

A follow–up of https://github.com/ckeditor/ckeditor5-alignment/issues/2.

@jodator jodator self-assigned this Nov 3, 2017
@jodator
Copy link
Contributor

jodator commented Nov 8, 2017

I've fiddled a bit with dropdowns in Google Docs and:

They have two major types of such dropdowns buttons:

  1. Button that upon click opens panel with options (may have state shown)
  2. Split button that have two active parts - button and split icon.

Ad. 1.

  • the button opens panel with other options (different alignment, size of panel)
  • the button may or may not represent "active" state
  • enter, up arrow, down arrow keys opens dropdown
  • arrows allows to cycle through buttons on opened dropdown
  • escape key close dropdown
  • does not have "active" state - it rather change icon to reflect formatting in selection (ie alignment)

Examples:
Gmail text alignment:
Gmail text alignment

Google docs line spacing:
selection_008

Ad. 2.

  • click on button executes command
  • click on arrow opens dropdown
  • enter on button executes command
  • enter or arrow up/dwon on arrow opens
  • arrows inside dropdown selects options (arrow left on first item closes dropdown - but for me this is confusing)

Examples
Command active in selection:
selection_009

Focus on button part:
selection_010

Focus on arrow part:
selection_011

Available options:
selection_006


As I can see we could go with similar approach and implement both as:

  • option 1 is simpler as it would group buttons and will change icon depending on which button is "on" (text alignment, headers).
  • option 2 is more usable where you can invoke selected command like in highlight feature where arrow would let you define which highlighter to use and button would either highlight selected text or remove it selection.

As for questions:

Should it have anything to do with the DropdownView?

For me it's direct child of dropdown view (at least option 1 that groups buttons).

How should toolbar config deal with the component?

There's already an issue for this with details in here.

The proposed way of grouping buttons to split button could be straightforward when:

  • only one button can be active at the time (see alignment feature)
  • whatever button is on it's icon is used for group
  • probably isEnabled should be a function of all inner buttons

I think that in MVP taking an alignment and header buttons grouping we should implement DropdownButtonGroup first. The other SplitButtonGroup for me can have a bit different requirements and could be handled in separate issue.

So the DropdownButtonGroup should:

  • accept collection of Buttons
  • should change icon to whatever button is active
  • could have config for inactive icon (or it should be defined which button is "default one")
  • it might be the first button in collection otherwise
  • on "enter"/"click" it should open dropdown to display all buttons

@jodator
Copy link
Contributor

jodator commented Nov 8, 2017

@oleq I'd made some prototype that binds isEnabled of child buttons to split button. Also the icon is bound to firts button that has isOn = true:

screencast 2017-11-08 17_07_06

I'd go with this solution (grouping buttonViews not models) to enable such configurations later on:

toolbar: [
    {
         type: 'dropdown',
         items: [ 'alignLeft', 'alignRight', 'alignCenter' ]
    }
]

as I understand this this would take toolbar items ( [ 'alignLeft', 'alignRight', 'alignCenter' ]) and create dropdown button from them. WDYT?

ps.: I haven't done much in terms of CSS/styling - I've just put some buttons as panel view children - so the alignment is as is.

@oskarwrobel
Copy link
Contributor

This might be a kinda related topic https://github.com/ckeditor/ckeditor5-ui/issues/124.

@jodator
Copy link
Contributor

jodator commented Nov 9, 2017

Another POC of using ButtonViews:

Adding 'alignments' toolbar component:

// This might be later refactored as toolbar config
componentFactory.add( 'alignments', locale => {
	const buttons = styles.map( style => { 
		componentFactory.create( AlignmentEditing.commandName( style ) );
	} );

	return createButtonDropdown( buttons, locale );
} );
// then use:
ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [ ArticlePluginSet, Alignment ],
		toolbar: [  'alignments' ] // or only buttons
	} )

Button behavior:

screencast 2017-11-09 12_38_14

Disabled state on image caption:

screencast 2017-11-09 12_39_23

@oleq
Copy link
Member Author

oleq commented Nov 9, 2017

As I can see we could go with similar approach and implement both as:

option 1 is simpler as it would group buttons and will change icon depending on which button is "on" (text alignment, headers).
option 2 is more usable where you can invoke selected command like in highlight feature where arrow would let you define which highlighter to use and button would either highlight selected text or remove it selection.

I agree 👍.

Plan

First stage

Let's focus on option1 first. We need it ASAP for alignment. TBH, it boils down to this old issue.

Issue to resolve here:

  • When using it for alignment, we expect the dropdown icon change as new alignment is chosen. But what about features that don't want anything to change when the items in the dropdown are selected (rough example below)?

    image

    How should the component react? Should it detect if items are buttons with icons and act accordingly? How to configure the icon of the dropdown button in the above "font size" case?

    To me, it sounds like a different component/configuration option. WDYT? (cc @Reinmar)

Second stage

Then let's extend it for headers. So developers can choose from:

  1. a big dropdown with selected heading name (current implementation)
  2. separate toolbar buttons (icons needed),
  3. a compact dropdown like in this issue.

Third stage

Option 2 will be needed by marker/highlight feature, so let's postpone it a little bit.

Configurability

To KISS and speed up the development, in MVP, there will be no such thing as

toolbar: [
    {
         type: 'dropdown',
         items: [ 'alignLeft', 'alignRight', 'alignCenter' ]
    }
]

because it means more work with the toolbar configuration and ComponentFactory class etc.. Just the following (names to be determined) provided by the factory:

config: {
	toolbar: [ 'headingsDropdown', 'bold', 'italic', 'custombutton' ]
}

or

config: {
	toolbar: [ 'headingX', 'headingY', 'headingZ', 'bold', 'italic', 'custombutton' ]
}

or

config: {
	toolbar: [ 'headingsCompactDropdown', 'bold', 'italic', 'custombutton' ]
}

@jodator
Copy link
Contributor

jodator commented Nov 9, 2017

When using it for alignment, we expect the dropdown icon change as new alignment is chosen. But what about features that don't want anything to change when the items in the dropdown are selected (rough example below)?

The demo above I've created with a new creature: ButtonGroup which simply gropus buttons and can have direction set (isVertical). It holds ButtonViews and should provide keystrokes handling for group (cycling, etc). Also it could be used as standalone (not in dropdown) - but maybe I'm overcomplicating things here).

Also I think that buttonDropdown might be "child" of different that dropdownList but both might have icon configurable to be either bound to active button or use "default one".

The defaultIcon might be usable also in a scenario when you define only two alignments (center and justify) but the default one left should be used as icon (either static either default when no button is active.

So in other words:

To me, it sounds like a different component/configuration option. WDYT?

Yes it's different component (current dropdown list feature).

@jodator
Copy link
Contributor

jodator commented Nov 9, 2017

@oleq: my dev ButtonGroup inside DropdownPanel with isVertical=false:

selection_003

Also a case for defaultIcon for dropdown to have.

oleq referenced this issue in ckeditor/ckeditor5-ui Dec 14, 2017
Feature: Initial implementation of the `ButtonDropdownView`. Closes #333.

Also: 
* Allowed vertical layout of the `ToolbarView` thanks to the `#isVertical` attribute.
* Implemented `ToolbarView#className` attribute.
* Implemented `DropdownView#isEnabled` attribute along with the CSS class binding.
oleq referenced this issue in ckeditor/ckeditor5-theme-lark Dec 14, 2017
Feature: Implemented styles for the ButtonDropdown (see: ckeditor/ckeditor5-ui#333). Fixed spacing issues with toolbar items wrapped to the new line. Closes ckeditor/ckeditor5#682.

Also:
* Various improvements to visual styles of the dropdowns.
* Migrated toolbar to CSS `flex`.
* Allowed vertical toolbars.
oleq referenced this issue in ckeditor/ckeditor5-heading Dec 14, 2017
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants