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

Create dropdown menu component #16631

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Create dropdown menu component #16631

wants to merge 9 commits into from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Jun 28, 2024

Feature (ui): Create dropdown menu component.

Part of #16311.


Other PRs

Focus change PR: #16642
Search integration PR: #16314
AI Integration PR: https://github.com/cksource/ckeditor5-commercial/pull/6213

Screens

⚠️ Search function is introduced in another PR.

Menu
Scrolling
RTL

Example usage

const view = new DropdownMenuRootListView( editor, [
  {
     menu: 'Menu 1',
     children: [
       // Initialize using definition object.
       {
          label: 'Menu Item 1.1',
          onExecute: () => { ... }
       },
       // Initialize using class instance.
       new DropdownMenuListItemButtonView( locale, 'Item' ),
       new ListSeparatorView( locale ),
       // Submenu initialization.
       {
         menu: 'Menu 1.1',
         children: [
           new DropdownMenuListItemButtonView( locale, 'Nested Item' ),
         ]
       }
     ] 
  } 
] );

// Somewhere later in the code.
view.factory.appendChild( {
  menu: 'Menu 2',
  children: [
    new DropdownMenuListItemButtonView( locale, 'Item 2' )
  ]
} );

// It is also possible to register custom menu using it's instance and modify it.
const customInstance = new DropdownMenuView( editor, 'Menu' );
customInstance.factory.appendChild( {
  menu: 'Sub menu',
  children: [
    new DropdownMenuListItemButtonView( locale, 'Item 3' ),
  ]
} );

// Walk over dropdown content. 
customInstance.walk( {
    Menu: () => { ... },
    Default: () => { ... }
} );

view.factory.appendChild( customInstance );
view.render(); 

Changes performed on menubar implementation

  1. Generate menus property dynamically instead of storing it as array class member. It is possible to register already created DropdownMenuView instance that was created outside of menu. Such instance might have items added or deleted depending on external integration logic (for example, adding button menu entry).

Example:

const customMenuInstance = new DropdownMenuView( editor, 'My custom menu' );
const view = new DropdownMenuRootListView( locale, [
  {
     menu: 'Menu 1',
     children: [
       // ... other menu items
       customMenuInstance
     ] 
  } 
] );

// ... later in the code ...
const newSubMenu =  new DropdownMenuView( editor, 'My submenu menu' );

newSubMenu.menuItems.addMany( [   
    new DropdownMenuListItemButtonView( locale, 'Your Searchable Item' ),
    new ListSeparatorView( locale ),
] );

// It's optional to use `appendMenuChildren` exported by root menu class. 
// New items can be added manually, without need to access root menu instance.
customMenuInstance.menuItems.add(
   new DropdownMenuListItemView( locale, menuInstance, newSubMenu)
);

// ... later in filter implementation ... 
// It should return 1 item that was added to `newSubMenu` because
// `DropdownMenuListItemButtonView` is automatically discovered while `tree` construction.
filterView.menuView.search( 'Your Searchable Item' );
  1. Added tree getter that allows to traverse through whole menu that allows for lookup for specific menu entries in tests, filtering menu or creating another instance of dropdown menu with mapped definition. The one nice side effect of such approach is the ability to create dump() functionality, which allows to easily serialize whole menu to simple pseudo-HTML form, which is super easy to test in other integrations.
const view = new DropdownMenuRootListView( editor, [
  {
     menu: 'Menu 1',
     children: [
       // initialize using definition object
       {
          label: 'Menu Item 1.1',
          onExecute: () => { ... }
       },
       // initialize using object
       new DropdownMenuListItemButtonView( locale, 'Item' ),
       new ListSeparatorView( locale ),
       // submenu initialization
       {
         menu: 'Menu 1.1',
         children: [
           new DropdownMenuListItemButtonView( locale, 'Nested Item' ),
         ]
       }
     ] 
  } 
] );

// Walk through menu and do some stuff on specific items.
view.walk( {
    Item: ( { node: { search } } ) => {
      if ( search.raw.includes( 'cats' ) ) {
         console.info( 'I see cat here...' ); 
      }
    }
} );

// Let's access tree and menu directly.
console.info( view.tree, view.menus );
  1. Horizontal menu bar and vertical positioning dropdown button positioning functions have been removed.
  2. DSL format of menu definition is simplified and groups menu attributes has been removed. Now it is possible to pass ListSeparatorView manually.
  3. Icons are optional, though it is possible to allocate space for missing if it is not present (by default it is disabled).
  4. Event delegation moved from root menu list to normal menu instances.
  5. Menu panel views are now mounted to ck-body-wrapper.

Drawbacks

  1. tree / menus getter calls might be expensive for huge menus (> 500 entries). It's not real case and at this moment there are no actions required to be performed in this scope. It is fixed by adding optional lazy initialization of menus.
  2. Scrolling in large sub-menus is missing.
  3. It might look awful on mobile.

Useful links and discussions

  1. https://mdn.github.io/dom-examples/popover-api/nested-popovers/
  2. https://blog.logrocket.com/use-css-anchor-positioning/
  3. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover
  4. Dropdown panel should not not be cut off when the editor is inside in an overflow: hidden host #5514 (comment)

Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

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

Some further cleanup required:

  • menu/definition directory - please put all things into one file, they are all mostly used only by the factory util, with I thing just one exception. Remove "definition" factory and move the singular file into menu/ directory.
  • Change the factory class name and file name to something like DropdownMenuFactory -- it does not create a definition, it creates a menu.
  • Drop everything related to "tree" utils, the whole directory. From what I checked, everything that is in there comes down to .menus getter. AFAICS, these utils are not used anywhere else (there is tree property too but it isn't used anywhere outside .menus, and there is .walk() which isn't used anywhere).
    • To build .menus I am pretty sure we can do it with a simple tree-search algorithm that uses while and an array to store results. We won't need to use several utils, types and files.
    • preloadAllMenus() seems not to be used anywhere besides tests (at least that what my IDE tells me).
    • Same with getCurrentlyVisibleMenusElements().

I wrote that maybe some of the utils may stay. I meant just menus, but I thought that menus include both menu views and button views. tree is not necessary. But we may need something like items (or buttons) -- all leaf items in the structure, flattened. So that we can operate on them when the editor state (e.g. selection) changes.

For items entries, we may need a "path" to them, i.e. an array of menus in which an item is. But I think that will be needed for the search only, and I purposely don't mention any search related things when discussing what should be dropped from this PR. As I mentioned multiple times, this PR should be only about the nested menus functionality.

@Mati365
Copy link
Member Author

Mati365 commented Jul 2, 2024

@scofalik

menu/definition directory - please put all things into one file, they are all mostly used only by the factory util, with I thing just one exception. Remove "definition" factory and move the singular file into menu/ directory.

👍🏻 done

Change the factory class name and file name to something like DropdownMenuFactory -- it does not create a definition, it creates a menu.

👍🏻 done

Same with getCurrentlyVisibleMenusElements().

At this moment, it's kinda crucial to get all DOM elements that belong to the menu. Without such helper, it's impossible to render dropdown menu inside dropdown. Check this place for additional PR that implements AI support: https://github.com/cksource/ckeditor5-commercial/pull/6213/files#diff-c3e2cfc5f2b64e23fc4bb25cdb3e76e1615a85a1fd0fac616a71d2271cb1456cR165

preloadAllMenus() seems not to be used anywhere besides tests (at least that what my IDE tells me).

As we discussed, it's used in search implementation right here. walk function cannot walk through a menu that has collapsed menu entries.

👍🏻 I moved this code to search PR.

To build .menus I am pretty sure we can do it with a simple tree-search algorithm that uses while and an array to store results. We won't need to use several utils, types and files.

While I agree that this solution is good enough in terms of making the tree flat, or simply iteration over such tree, it makes it difficult to perform search using such approach. We need to know when we enter to the node, when we leave, to construct a proper search tree. Please check this file. I used API that is quite similar to ast-types, so I think it's quite normal way to solve that kind of the problems.

⚠️ Partially done. I simplified the tree walker and moved more sophisticated logic to search PR.

and there is .walk() which isn't used anywhere

It's used especially in testing, and AI integration to enable or disable menu entries of specific type. It's used here.

For items entries, we may need a "path" to them, i.e. an array of menus in which an item is. But I think that will be needed for the search only, and I purposely don't mention any search related things when discussing what should be dropped from this PR. As I mentioned multiple times, this PR should be only about the nested menus functionality.

⚠️ So that's why the flatten tree method was added :)

tree is not necessary

❌ It is necessary, and it's not only my opinion. It was originally added in the menu bar (here) for testing purposes. It allows us to compare tree structures created by definition quickly. At the same time, I agree that there are a lot of other, simpler approaches, to do that but I believe that it's the only one that is the most elegant one because filtering, flattening, and other tree-things can be done using the most common algorithms that are basically copy & paste from other implementations.

@Mati365 Mati365 requested a review from scofalik July 2, 2024 15:01
@oleq oleq removed their request for review July 25, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants