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

Popup menu search #916

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Popup menu search #916

wants to merge 2 commits into from

Conversation

philippfromme
Copy link
Contributor

  • adds ability for popup menu providers to provide custom search
  • default search sorts matches by location (when searching for "foo", "foo bar" will appear before "bar foo")

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jun 5, 2024
@philippfromme
Copy link
Contributor Author

Example usage here: https://github.com/camunda/camunda-bpmn-js/pull/367/files#diff-1abbb6d8413b72aac12009eae34de187cdc46f9c9eeb5daad527e5c7c43a8df8R23

Not entirely shure if this is the way to do it or wether we should have separate providers for search similar to: https://github.com/bpmn-io/diagram-js/blob/develop/lib/features/search-pad/SearchPadProvider.ts

@@ -557,6 +570,15 @@ PopupMenu.prototype._getEmptyPlaceholder = function(providers) {
};


PopupMenu.prototype._getFindEntries = function(providers) {
const provider = providers.find(
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely shure if this is the way to do it or wether we should have separate providers for search similar to: https://github.com/bpmn-io/diagram-js/blob/develop/lib/features/search-pad/SearchPadProvider.ts

We should probably use the existing pattern; define a PopupMenuSearchProvider to be overridden as we see fit.

Copy link
Member

Choose a reason for hiding this comment

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

Even better: Generalize search (can be a follow-up):

  • Given a set of objects, search fields primary: [a], secondary: [b], other: [c] (in order of priority)
  • This way we need to customize the search only once, and re-use across places where it is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the idea would be for both the popup menu and the global search to do:

// search available through injector, can be overridden, similar to `translate`
const searchFn = injector.get('search');

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is what I'm thinking.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to sketch this next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think this could be a good direction. 👍🏻

@philippfromme philippfromme added the ready Ready to be worked on label Jun 25, 2024 — with bpmn-io-tasks
@philippfromme philippfromme removed the in progress Currently worked on label Jun 25, 2024
@philippfromme philippfromme force-pushed the popup-menu-search branch 2 times, most recently from e62a656 to 5f694ad Compare July 18, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready to be worked on
Development

Successfully merging this pull request may close these issues.

None yet

2 participants