-
Notifications
You must be signed in to change notification settings - Fork 198
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
feat: restore focus on the canvas when properties panel loses focus #1093
Conversation
@@ -158,6 +163,11 @@ export default class BpmnPropertiesPanelRenderer { | |||
return event.providers; | |||
} | |||
|
|||
_restoreCanvasFocus() { | |||
const canvas = this._injector.get('canvas'); | |||
canvas && canvas.restoreFocus && canvas.restoreFocus(); |
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.
canvas && canvas.restoreFocus && canvas.restoreFocus(); | |
// support diagram-js@15+ | |
if (canvas.restoreFocus) { | |
canvas.restoreFocus(); | |
} |
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.
canvas
will always be there (or otherwise inject.get('name')
would have blown up already.
If you want canvas to be optional, do injector.get('canvas', false)
.
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.
For a moment I thought we could simply hook into our focus*
events, until I realized we don't have those...
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.
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.
Ok, so my organized thoughts on this PR, looking at how it is built:
It feels odd that we don't have proper eventBus
events to hook into to integrate the focus change. Like a house we want to build "foundations" as we add new features (or balconies), and I'd assume that a propertiesPanel.focus.changed
event (similar to the new canvas.focus.changed
event) allows me to hook in the behavior.
I sketched that via this branch, which also contains fixes for the broken focus change within the panel that I reported.
UX wise, I suggest you to do always do some exploratory testing. It would be odd if we introduce silly bugs because we did not verify that the thing we fix works in the large scheme of things.
Closing this pull request as we want to go with a follow-up by Nico - #1094 |
Proposed Changes
Related to https://github.com/bpmn-io/internal-docs/issues/1081
Related to camunda/camunda-modeler#4620
Make use of the new
diagram-js@15
Canvas#restoreFocus
API to focus back on the canvas when properties panel loses focus.In addresses a case explain in this comment, when user briefly interacts with properties panel without focusing any input. In that case, we want the focus to stay on the modeling canvas.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}