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.

Sign up for GitHub

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

Jump to bottom

Search improvements #2187

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Search improvements #2187

merged 3 commits into from
Sep 12, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Jun 3, 2024 •
edited by nikku
Loading

Improves search by

  • sorting matches by location (when searching for "foo", "foo bar" will appear before "bar foo")

brave_noS564awP1

Depends on bpmn-io/diagram-js#913

Closes #2189

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jun 3, 2024
@philippfromme philippfromme mentioned this pull request Jun 3, 2024
Merged
@philippfromme philippfromme force-pushed the search-improvements branch from 2635f40 to 8092da6 Compare June 5, 2024 09:03
@philippfromme philippfromme marked this pull request as ready for review June 5, 2024 14:05
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jun 5, 2024
@philippfromme philippfromme force-pushed the search-improvements branch from 8092da6 to 5200d17 Compare June 5, 2024 14:21
@barmac
Copy link
Member

barmac commented Aug 21, 2024

@philippfromme There are conflicts to resolve in this PR

barmac
barmac reviewed Aug 21, 2024
View reviewed changes
lib/features/search/BpmnSearchProvider.js Outdated
secondaryTokens: matchAndSplit(element.id, pattern),
element: element
};
return !isLabel(element) && element !== rootElement;
Copy link
Member

Choose a reason for hiding this comment

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

👍 for helper usage

@barmac
Copy link
Member

barmac commented Aug 21, 2024

Let's fix the conflict, and it is ready to be merged.

@nikku
Copy link
Member

nikku commented Aug 21, 2024

I checked this PR once again, UX wise, as I previously lost one of my reviews 🙈

One thing I found is that clicking anywhere on the canvas with search open would reset the viewport to the previous viewbox. I believe if the user clicks anywhere (closing search), then they want to commit the current viewbox, and do not expect it to "jump back". Things are, of course, different if they press ESC ("GET ME OUT OF THERE").

Also I found that sometimes focussing of elements is odd (cf. jumping when selecting different sub-processes that would actually be "in view".

Both visualized in the following recording:

capture ngU8IG_optimized

nikku
nikku requested changes Aug 28, 2024
View reviewed changes
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Cf. #2187 (comment).

Let's consider if we want to fix the last bug.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 28, 2024
abdul99ahad
abdul99ahad previously requested changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@abdul99ahad abdul99ahad left a comment

Choose a reason for hiding this comment

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

Merge conflicts in the PR. Please resolve them.

jarekdanielak pushed a commit to bpmn-io/diagram-js that referenced this pull request Aug 28, 2024
@philippfromme
feat: search improvements
7d41f25 
Related to bpmn-io/bpmn-js#2187
@philippfromme
Copy link
Contributor Author

@nikku @abdul99ahad I resolved the merge conflicts. This pull request is ready to be reviewed. The remaining issues mentioned by @nikku need to be fixed in diagram-js. I'll work on that.

@philippfromme philippfromme requested a review from nikku September 10, 2024 12:14
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Sep 10, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Sep 10, 2024
philippfromme added a commit to bpmn-io/diagram-js that referenced this pull request Sep 10, 2024
@philippfromme
fix: do not restore cached when closing on click outside
ab6969a 
Related to bpmn-io/bpmn-js#2187
@philippfromme philippfromme mentioned this pull request Sep 11, 2024
Merged
4 tasks
philippfromme added a commit to bpmn-io/diagram-js that referenced this pull request Sep 11, 2024
@philippfromme
fix: do not restore cached when closing on click outside
605c889 
Related to bpmn-io/bpmn-js#2187
nikku pushed a commit to bpmn-io/diagram-js that referenced this pull request Sep 12, 2024
@philippfromme @nikku
fix: do not restore cached when closing on click outside
5312291 
Related to bpmn-io/bpmn-js#2187
philippfromme and others added 3 commits September 12, 2024 14:11
@philippfromme @nikku
chore(search): replace deprecated method
5bfe39b
@philippfromme @nikku
feat(search): sort matches by location
1654968 
* simplify implementation
* take location into account when sorting
@nikku
deps: update to [email protected]
09d8711 
Improves various aspects of the built-in search.

Closes #2189
@nikku nikku force-pushed the search-improvements branch from 44d6303 to 09d8711 Compare September 12, 2024 12:13
nikku
nikku approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Great stuff.

@nikku
Copy link
Member

nikku commented Sep 12, 2024

Updated on top of latest DEV branch + pulled in [email protected].

@nikku nikku merged commit c1fc4e8 into develop Sep 12, 2024
12 checks passed
@nikku nikku deleted the search-improvements branch September 12, 2024 12:23
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 12, 2024
@nikku nikku mentioned this pull request Sep 12, 2024
Closed
2 tasks
@nikku
Copy link
Member

nikku commented Sep 12, 2024

Did some integration testing on the released changes, and identified potential follow-ups:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barmac barmac barmac left review comments

@nikku nikku nikku approved these changes

@marstamm marstamm Awaiting requested review from marstamm

@abdul99ahad abdul99ahad Awaiting requested review from abdul99ahad

Assignees

@philippfromme philippfromme

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Diagram search should only commit navigation changes on ENTER
4 participants
@philippfromme @barmac @nikku @abdul99ahad