Dropdown family (possibly) needs architecture refactoring

[oleq]

ATM there are 2 base classes:

DropdownPanelView
DropdownView

and 3 helpers combining them with ToolbarView, ListView, etc.:

createDropdown()
createButtonDropdown()
createListDropdown()

However, the new Highlight feature we're implementing requires a split button,
which does not actually fit that well into the family because it actually comes with 2 buttons in the toolbar:

• main one which applies the highlight,
• the side one which opens the dropdown with styles.

The goal: We need to somehow make it possible to create such a dropdown supporting all events like execute and attributes like isOpen using just base classes and helper functions.

JS requirements

ATM DropdownView accepts button and panel as constructor arguments. Then it extends button's template and starts reacting to ButtonView#execute by opening the panel.

In case of the split button, the situation is quite different. The left (main) part of the split button usually executes some commands (e.g. applies last used highlight), while the right button acts as the panel opener. So, in fact, the main button is "extra" here, something new.

With the current DropdownView architecture, it is not possible to pass such split–button and handle it without lots of hacking.

CSS requirements

There are 2 dropdown styles in Lark:

1. dropdown with text which has background and border,
2. dropdown without text (just an icon), borderless.

The later is ATM supported only by createButtonDropdown() but it should become a standard feature of the DropdownView, I guess. Every dropdown should come with that option to use just an icon or just a text, which obviously should be reflected in a class of the element:

<!-- e.g. for headings feature -->
<div class"ck-dropdown ck-dropdown_with-text"></div>

vs.

<!-- e.g. for alignment feature -->
<div class"ck-dropdown"></div>

The class corresponds to ck-button_with-text.

Note: While the styling could change soon and all dropdowns might look the same, this still must be differentiated.

Possible solutions

Allow passing the split button directly into DropdownView constructor

That could work if we figured an interface for such split button which makes sense. Note that both parts of the split button have isEnabled attribute and they both fire execute when clicked.

1. How to allow the DropdownView to listen to the "right" execute and open the panel?

2. How to let the DropdownView knew which of the two split buttons is enabled to activate the keyboard support?

Maybe it should be createDropdown()'s job, limiting the DropdownView to just being a dull container?

Let's just create a new kind of DropdownView for the split button purposes

It's quite simple but it will duplicate tons of code (and tests) doing exactly the same just to allow a fancy button. It does not make much sense to me, IMO the new feature should remain in the base DropdownView family.

???

Post-refactoring

Some naming issues should probably be resolved, like this one because we should name dropdown helpers by the kind of content it contains:

createButtonDropdown() -> createToolbarDropdown()

Which name to use for the "split dropdown" then? createSplitButtonDropdown? Probably yes, because it should be able to contain any sort of content. Or maybe we should figure out the new set of helpers according to the following methodology?

button type (single vs. split) 
+ 
content type (list vs. toolbar vs. empty) 
= 
result dropdown

like

create[Split|][List|Toolbar|]Dropdown()

I'm not sure how to resolve that. Which helpers should exist and to what extent things should be "helperized"?

cc @Reinmar @jodator @oskarwrobel

[jodator]

TL;DR:

• define basic event interfaces for button / model / content interactions
• create base helper methods for building dropdowns
• tie button appearance to model only in base methods
• move some binding to helper methods or features

---

After reading requirements and having possible combinations I don't see helper only solution nice. Creating helpers for every possible solution doesn't look maintainable.

The process of creating a toolbar dropdown goes like described above:

1. button type
2. content type
3. interactions

Resulting dropdown.

Maybe we should think which interactions should be defined by UI and which by features?
The common dropdown behaviors are:

• open dropdown
• cycle through list items
• cycle through toolbar items
• focus content on open dropdown
• close dropdown on execute
• close dropdown on esc

Those should be provided as 'event' interfaces of two required elements (button + contents) so one could pass any compatible View to very base 'createDropdown()` helper.

Some feature specific are:

• Change title of toolbar button on active command value.
• Change toolbar icon to active button inside toolbar icon.
• Change icon of toolbar icon on command value.
• Change executed command of split button on command value.
• When the toolbar button isOn.
• When the toolbar button isEnabled.

Those are mostly controller by the DropdownModel so features should bind as they wish model's attributes to required behavior.

The building DP process might look like this:

const model = new Model();

const button = createDropdownButton( model, locale );
const dp = crateDropdown( model, button, locale );

addListToDropdown( dp, listItems );
// OR
addToolbarToDropdown( dp, buttons );

// This will add 'close', 'open' behaviors.
addDefaultBehavior( dp );

Now having some common toolbar configurations we can have creator methods similar to current createListToolbar() or createButtonDropdown().

For instance I would remove current icon binding in dropdown with buttons that is a bit whacky (it's either staticIcon or using defaultIcon) and move it to either Alignment feature only (changing icon upon selection change) or create a helper method that will enable such behavior on created dropdown.

const model = new Model();

const button = createDropdownButton( model, locale );
const dp = crateDropdown( model, button, locale );
addToolbarToDropdown( dp, buttons );
addDefaultBehavior( dp );
bindIconToActiveButtonIcon( dp );

As an alternative we could use Builder interface instead of helper methods:

const dp = new DropdownBuilder( model, locale )
    .withButton()
    .openingToolbarDropdown()
    .bindingIconToActiveButton()
    .build(); // this would add default behavior as it is always required

---

Some CSS cleanup:
Right now some CSS are tied to 'dropdown' but should be moved to specific element.

One example is previous refactoring of when to add background color to dropdown button - styling button__with-text instaed every dropdown button (as only Headings reauired that).

Another example is dropdown triangle added to every dropdown_button - which was trouble some for SplitButton as I initially had to hide that triangle from main button and add it once again to SplitButton's arrow button. So depending on button type (normal|split) appropriate helper method should add CSS class (like dropdwon_arrow) to appropriate element.

[jodator]

ps.: I would love to make it together with SplitButton and Highlight feature as the initial code for adding split button dropdown for highlight feature is a mess right now and requires cleanup anyway.

[oleq]

The idea sounds good to me. Creating dropdowns is a pain ATM. What we need is a tree of possible combinations to understand the scope:

Buttons

• Button with text,
• Button with icon,
• Button with text and icon,
• Any of the above + "split" button

Panel

• We have a single panel which accepts some content

Types of content in the panel

• Nothing
• List
• Toolbar

Did I miss anything?

Behaviours

• Focus of the button – users must be able to navigate with the keyboard inside the dropdown button(s)
• Focus of the panel – pressing down arrow must move the focus to the first child of the panel (list.focus() or toolbar.focus()) and such child is responsible for taking care of the rest.
• Changing button icon – this should be handled by the feature (via the model), IMO.
• Changing button text – same as above.
• Changing button state (#isEnabled, #isOn) – same as above.
• Executing command upon main (left) button in the split scenario – event fired, but handled by the feature.

Another example is dropdown triangle added to every dropdown_button - which was trouble some for SplitButton as I initially had to hide that triangle from main button and add it once again to SplitButton's arrow button.

Since #645, the arrow is an instance of IconView as a child of the dropdown. So managing it will be easier IMO.

[jodator]

@oleq: Yep that summarizes my thoughts on the behavior and responsibility split.

[Reinmar]

I've tried reading through your discussion but I see that it will be very hard for me to give a more detailed feedback. However, I see that you're doing a great job analysing these components, so I'll focus on some general aspects.

As an alternative we could use Builder interface instead of helper methods:

We're right now killing the builder idea in the engine. It turned out that you can easily do stuff by using the builder but if you need anything more custom or slightly different, then you can't use the builder at all.

Therefore, a set of utils seems to be a safer solution. However, the trickiness here lays in proposing meaningful and useful utils. E.g. I proposed things like closeOnClickOutside() in the past and I'm still unsure whether this is good :D. Isn't it too low level? Shouldn't it be more like actAsFloatingPanel() which could then not only handle clicking but also escaping focus, dnd, touch devices, etc.?

So, I'd propose the following goals for this task:

• don't go too low and don't create too many (too granular) utils,
• don't go too high and don't create too specialised utils,
• don't overcomplicate – focus on the case that we have or expect to have in a reasonable future – leave edge cases for manual resolution.