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

Use search #2235

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

Use search #2235

wants to merge 2 commits into from

Conversation

nikku
Copy link
Member

@nikku nikku commented Sep 13, 2024

Proposed Changes

This plugs into the new search utility shipped with bpmn-io/diagram-js#916 / [email protected].

Some observations:

  • The search syntax and what is required by search-pad are incompatible, and require manual conversion. This is inconvenient. I do regard the new format (token = { value, matched: boolean }) to be supperior. Maybe we could make SearchPadProvider compatible with it?

  • search only returns tokens where they exist. This is inconvenient and requires me to manually check if tokens are return. Example: I test objects of the form { name: string } for the key label, and expect the result to contain { tokens: { label: [ ... ] } } regardless of whether any of the objects had a label property. Today, undefined is returned and must be accounted for.

  • We only return matched tokens, and do not return what did not match. This leads to changes in the output (it does no longer show previously visible information (i.e. the ID, whether or not it matched):

    capture wdEgza_optimized

Overall we can see that this is quite an improvement, removing the heavy matching / tokenization from implementors. Great job 🎖️

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

Depends on bpmn-io/diagram-js#932

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Sep 13, 2024
This ensures we build upon the new search abstraction, built via bpmn-io/diagram-js#916.
@nikku
Copy link
Member Author

nikku commented Sep 13, 2024

Test failures are due to the fact that we break the existing contract: We don't return un-matched tokens anymore.

@nikku
Copy link
Member Author

nikku commented Oct 2, 2024

Depends on bpmn-io/diagram-js#932 to be fixed.

@nikku
Copy link
Member Author

nikku commented Oct 2, 2024

Also depends on / users would benefit from restoring fuzzy search.

@nikku nikku requested a review from lmbateman October 10, 2024 14:03
@lmbateman
Copy link

I'm not sure I understand the prior behavior and its problems well enough to comment. Any chance I could get a demo with some explanation? Thank you!

@nikku
Copy link
Member Author

nikku commented Oct 10, 2024

@lmbateman I'll give you a demo once I'm close to merging this, but it is important to me to mention you, so you're aware of UX improvement in search (popup menu, and global search).

@nikku nikku added the ready Ready to be worked on label Oct 11, 2024 — with bpmn-io-tasks
@nikku nikku removed the in progress Currently worked on label Oct 11, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants