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

Feel Popup event support #287

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Feel Popup event support #287

merged 3 commits into from
Sep 22, 2023

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Sep 22, 2023

Related to bpmn-io/bpmn-js-properties-panel#974

This provides the capability to hook into the feel popup lifecycle as

  • feelPopup.opened
  • feelPopup.closed
  • feelPopup._open
  • feelPopup._close
  • feelPopup._isOpen

Furthermore, this adds a feature service that integrators can use to interact with the popup. Cf. to this example on how to use it. I didn't add particular test coverage for this module in this project similar to debounceInput as I saw we do not have a proper didi-like test setup in the core framework.

const feelPopup = bpmnjs.get('feelPopup');

console.log('is open', feelPopup.isOpen());

feelPopup.close();

feelPopup.open(someElement, { type: 'feel' }, elementToRestoreFocusAfterClose);

Happy to receive your feedback ❤️ @bpmn-io/modeling-dev @bpmn-io/hto-dev

Niklas Kiefer added 2 commits September 22, 2023 12:35
* `feelPopup._open`
* `feelPopup.opened`
* `feelPopup._close`
* `feelPopup.closed`
* `feelPopup._isOpen`
return isOpen();
};

eventBus.on('feelPopup._close', handleClose);
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep backwards compatibility then we'd need to make eventBus optional.

nikku
nikku previously approved these changes Sep 22, 2023
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.

This looks great. One last question: Do we need to make eventBus an optional dependency at the core?

@nikku nikku dismissed their stale review September 22, 2023 11:35

Should be a comment

@pinussilvestrus
Copy link
Contributor Author

I liked the null-object pattern usage here, so I used that via 0abe5d7. 🏆

I believe that is okay, as eventBus is optional in the parent PropertiesPanel component as well (at least from the JSDocs 🙂)

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.

@pinussilvestrus
Copy link
Contributor Author

I'd take over releasing these bits and open up bpmn-io/bpmn-js-properties-panel#975 if that's okay to you.

@nikku
Copy link
Member

nikku commented Sep 22, 2023

That's great, thank you :)

@pinussilvestrus pinussilvestrus merged commit 3ce7285 into main Sep 22, 2023
10 of 11 checks passed
@pinussilvestrus pinussilvestrus deleted the 974-feel-popup-events branch September 22, 2023 11:59
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants