-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add search support to dropdown menus #16314
base: ck/16311-core
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Out of diff range and nitpick comments (6)
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts (6)
50-66
: Add documentation to theofConfig
method to explain its parameters and behavior.
71-95
: Add documentation to the_createMenu
method to clarify its purpose and the parameters it accepts.
98-139
: Add documentation to the_createMenuItems
method to describe its functionality and parameters.
145-181
: Add documentation to the_createMenuItemContentFromFactory
method to explain its behavior and parameters.
187-213
: Add documentation to the_registerMenuTree
method to detail its functionality and parameters.
218-229
: Add documentation to theregisterMenu
method to explain its behavior and parameters.
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/ckeditor5-ui/src/bindings/clickoutsidehandler.ts (1)
[!TIP]
Codebase VerificationSeveral files in the codebase still do not utilize the new
options
parameter inclickOutsideHandler
. It's important to review whether this parameter should be used in these instances or if its current usage is intentional.
packages/ckeditor5-ui/src/toolbar/block/blocktoolbar.ts
packages/ckeditor5-ui/src/menubar/utils.ts
packages/ckeditor5-ui/src/dropdown/menu/utils.ts
packages/ckeditor5-ui/src/dropdown/utils.ts
packages/ckeditor5-table/src/tableproperties/tablepropertiesui.ts
packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesui.ts
packages/ckeditor5-mention/src/mentionui.ts
packages/ckeditor5-link/src/linkui.ts
packages/ckeditor5-image/src/imagetextalternative/imagetextalternativeui.ts
packages/ckeditor5-image/src/imageresize/imagecustomresizeui.ts
Analysis chain
Line range hint
29-55
: LGTM! Ensure the newoptions
parameter integrates well with other parts of the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of the `options` parameter in `clickOutsideHandler` with other parts of the system. # Test: Search for usages of `clickOutsideHandler` and check if `options` is being passed correctly. rg --type typescript $'clickOutsideHandler'Length of output: 77
Script:
#!/bin/bash # Description: Verify integration of the `options` parameter in `clickOutsideHandler` with other parts of the system. # Test: Search for usages of `clickOutsideHandler` and check if `options` is being passed correctly. rg --type ts 'clickOutsideHandler'Length of output: 2782
Script:
#!/bin/bash # Description: Extract the lines of code where `clickOutsideHandler` is called to check if the `options` parameter is being passed correctly. # Extract the relevant lines from each file where `clickOutsideHandler` is called. rg --type ts --context 2 'clickOutsideHandler\('Length of output: 4937
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Out of diff range and nitpick comments (5)
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenufilteredlistview.ts (3)
17-17
: Add a description for the TODO comment to clarify pending tasks or remove if not applicable.
21-23
: Add a description for the TODO comment to clarify pending tasks or remove if not applicable.
31-50
: Ensure proper documentation for the constructor and its parameters for better maintainability.packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts (2)
25-27
: Add a description for the TODO comment to clarify pending tasks or remove if not applicable.
29-48
: Ensure proper documentation for the constructor and its parameters for better maintainability.
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenufilteredlistview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenurootlistview.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenulistfilteredview.ts (1)
17-23
: Complete the TODO comments with appropriate descriptions or remove them if they are no longer needed.packages/ckeditor5-ui/src/dropdown/menu/utils.ts (1)
45-64
: The implementation oftoggleMenusAndFocusItemsOnHover
correctly handles menu toggling on hover. However, consider resolving the TODO comment regarding whether to focus on hover, as this can affect user experience.
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenulistfilteredview.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
packages/ckeditor5-ui/src/dropdown/menu/utils/dropdownmenulookup.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattenmenuviews.ts (1)
18-74
: The functioncreateTreeFromFlattenMenuViews
is well-implemented with clear logic for constructing a tree from a flat list of menus. However, it heavily relies on non-null assertions (e.g.,menu.buttonView.label!
andmenu.panelView.children!.first
). It would be beneficial to add a comment clarifying that these values are expected to be non-null as per upstream guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
packages/ckeditor5-ui/src/dropdown/menu/search/tryremovedropdowntreechild.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/tryremovedropdowntreechild.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/tryremovedropdowntreechild.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
packages/ckeditor5-ui/src/dropdown/menu/search/groupdropdowntreebyfirstfoundparent.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
packages/ckeditor5-ui/src/dropdown/menu/definition/dropdownmenudefinitiontypings.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/definition/dropdownmenudefinitionparser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/createtreefromflattendropdownmenuslist.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/search/walkoverdropdownmenutreeitems.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/ckeditor5-ui/tests/dropdown/menu/search/filterdropdownmenutreebyregexp.js (1)
1-4
: Ensure the license header is up-to-date.Verify that the license header is current and matches the project's standard format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
packages/ckeditor5-ui/tests/dropdown/menu/filterview/dropdownmenulistfoundlistview.js
Outdated
Show resolved
Hide resolved
0d1c741
to
89f26df
Compare
Thanks for providing an overview of changes introduced by the PR. I am in the middle of the review but I have some questions about what you wrote.
Do we need this for any specific functionality right now? The two features that for sure will use the new dropdowns are Merge fields and AI assistant. Both features have a static list of categories and items. Much of what you wrote and explained is related to adding items dynamically. It wasn't in the scope of this component and maybe it can be simplified. One, because it's easier to review and close a PR when there's less code. Two, because we don't want to provide a public API too hastily. Three, because there's a high chance this code will be unified and/or refactored anyway in a few quarters. And four, because it might be beneficial in another area:
We have a real case for this, as we have reports from some users that they are looking to use as many as hundreds of merge fields. So, I am concerned about performance here. |
I pushed few modifications.
|
d0c9610
to
c935f34
Compare
Follow-up ticket: #16613 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the PR. Despite committing quite some time to it, I'd say that was just only initial review. I haven't touched many of the included files.
I believe this PR is overwhelmingly big and it includes many changes/functionalities, which makes it impossible to fully grasp and analyze it. It's hard to build confidence with such PR. It's also overengineered in a few places.
One thing that bothers me is that the current menu bar implementation is a bit overcomplicated too (with the amount of classes it provides). Since this PR was supposed to base on the menu bar, it inherited that overcomplexity.
In order to ship some functionality in a reasonable time, I propose a fixing plan for this PR. I guess the only other option would be to start over clean.
First of all, let's remove all stuff related to search, search input, filtered view, related utils/mechanisms, and CSSes. Let's focus on introducing just the component for the nested dropdowns, similar what we have in the menu bar. Things that are directly related to the core functionality, like e.g. creating panels in body-container to allow max-height, they should stay. We will add search capabilities in next step.
Second, let's try to remove all the typings, guards, utils and "it will be useful"-things that are not necessarily needed. If something is used just in one class, move it to that class file and have it as a local function / private method. I think that some of the utilities can be helpful, like e.g. access to the flattened list of items in the whole dropdown structure. This can stay if it is still used by anything in the code (after other simplifications are introduced). But in general, let's keep these utils at minimum.
Third, following above, let's try to have less files total. I hope we will have less utils and they will all go to one file (dropdown/menu/utils.ts
). Types or events declarations can go to the files they are strictly related to. If we really need to, we can also have dropdown/menu/typings.ts
. Menu bar has 9 files total, I hope we can achieve similar quantity here.
Fourth, implement other simplifications I mentioned in comments to this review.
Fifth, I'll repeat again, let's keep the amount of changes to the minimum.
I wrote that I made only "initial review". I didn't review search and filter view related files at all. In the rest of the files, I mostly commented on things I didn't understand or I felt are overcomplicated or maybe not necessary, but I didn't focus enough to make sure that everything is bug-free etc. So there will have to be another review, more in-depth, hopefully the new PR will be 1/3rd of the size of this.
Some other tips:
"Old" dropdowns, which we have in toolbars, they allow to have everything in them. But in this case, we are implementing a solution which is very well defined and it does not need to be very open-ended. We just need lists with two things: buttons that perform actions, and buttons that open more lists. We should "live" only in dropdown/menu
space. Of course, extending some base classes like View
or ListView
(if it helps) but we can be specific on what we allow in "items". Basically, when you iterate over items, you should always have only two options: "leaf" button, or "dropdown-trigger" button. MAYBE separator. Finally, "menu" dropdowns, do not need to replace current dropdowns, so let's focus on the two use cases we have, i.e. AI Assistant and Merge Fields.
When I think about AI Assistant and Merge Fields, how would I like to create the component for them, it's something that could be simplified to:
// Config value, post-processed to fit `DropdownMenu` definition structure.
const definition = [
{
// Merge field category.
label: 'Customer',
id: 'customer',
items: [
// Button, upon clicking inserts a merge field.
{
label: 'Customer name',
id: 'customer.name' // Merge field name.
},
{
label: 'Customer address',
id: 'customer.address'
},
// ... More merge fields
]
},
// ... More merge fields categories
// Could have top-level merge fields too.
];
// When leaf button is clicked, `onExecute` is called with its id.
const onExecute = buttonId => editor.execute( 'insertMergeField', buttonId );
// Create dropdown menu structure. This could be called `DropdownMenuView` to keep the convention,
// or if we don't want to have such heavy view, it could be created by an utility function/factory.
const mergeFieldsDropdown = new DropdownMenu( editor.locale, definition, onExecute );
// Create toolbar button:
const button = new Button( ... );
// Show dropdown when button is clicked, simplified, of course, but that's outside of the scope of this PR.
button.on( 'execute', () => {
mergeFieldsDropdown.isOpen = true;
} );
I would also try to get rid of the "intermediate" classes like DropdownMenuListItemView
or DropdownMenuPanelView
, cause it would seem to me that all we need is: button and list. MAYBE separator. I am not sure if that's 100% possible, but I'd try to do it.
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenulistitembuttonview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/dropdownmenulistitemview.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/definition/dropdownmenudefinitiontypings.ts
Outdated
Show resolved
Hide resolved
this.on<DropdownMenuChangeIsOpenEvent>( 'menu:change:isOpen', ( evt, name, isOpen ) => { | ||
clearTimeout( closeTimeout ); | ||
|
||
if ( isOpen ) { | ||
this.isOpen = true; | ||
} else { | ||
closeTimeout = setTimeout( () => { | ||
this.isOpen = this.menus.some( ( { isOpen } ) => isOpen ); | ||
}, 0 ); | ||
} | ||
} ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish there was a better way to handle that than timeouts, which are always some kind of a hack. Do we need to use timeouts because of some async events, like blur and focus deep below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scofalik It's a hack that was copied menu, but I believe it's some sort of debounce which reduces the amount of the items iterated using .some()
on large menus, so I decided to keep it.
packages/ckeditor5-ui/src/dropdown/menu/utils/dropdownmenubehaviors.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/utils/dropdownmenubehaviors.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/utils/dropdownmenubehaviors.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-ui/src/dropdown/menu/utils/dropdownmenubehaviors.ts
Outdated
Show resolved
Hide resolved
FYI: I removed some of the comments that got to the review accidentally and I haven't seen them because the code changed (I started the review a long time ago...). |
Just to make it clear: I wouldn't go fixing the comments one after another. E.g. some of the files/classes that I commented may not end up in the final PR after simplifications, or they may got refactored to the point that my comment is outdated. So, first make the plan what is to be kept, what should be refactored, and what should be removed. Then see if my comments still apply. You can start over if that's easier for you and copy-paste parts of the code from this branch. |
Size of PR
Yes, it's because it's menu with some additional features like search integration and scrolling support, which are not present in menubar alone. It added a bit of complexity. Keep in mind that the current implementation of menubar is approximately 7k lines long, without search functionality but with some other enhancements, so the current solution proposed in this PR is compact even if it has a lot of examples in the comments. I merged some tree utils together, there should be fewer files. Remove search functiontl;dr I kept this branch, and it's still being used in AI and created additional PR with core functionality.
There are three problems with that: Dropdown menus feature is designed around searchI designed a whole solution to keep implementation of search as small as I can, so basically the whole search feature is present in
which uses these helpers:
So around 20% of this PR will be reduced and of course, we can remove them, but I'm quite sure that they will remain in identical form finally. There is no way to make this PR 1/3 of it's size. To make it clear: removal of search will not result in removal There are pending changes in merge tagsThere are few conflict places that modified the behavior of dropdowns in similar way the dropdown menus did. Like this one:
I believe that there will be merge conflicts, and we have to resolve it as fast as we can. Splitting PR into two will delay it.
I'd suggest not doing that. While I get argument about "file invasion" I also see that testing and debugging huge "util file of truth" is even worse. I added a lot of examples and docs explaining what these helpers are doing, and I think that "util file of truth" will be too huge to maintain. At this moment I removed not necessary guards and typings and moved many of them to destination files. There is pending PR that unifies
|
8c62b71
to
3c2c2ed
Compare
f89449c
to
afd0447
Compare
Feature (ui): Add search support to dropdown menus.
Part of #16311.
Related PRs
Focus change PR: #16642
Base dropdown component PR: #16631
AI integration PR: https://github.com/cksource/ckeditor5-commercial/pull/6213
Screens
Further context
This PR adds search input to whole dropdown menus implementation. It reduced the size of main PR.