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

feat(popup-menu): fire opened/closed events #718

Merged
merged 2 commits into from
Dec 8, 2022
Merged

feat(popup-menu): fire opened/closed events #718

merged 2 commits into from
Dec 8, 2022

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Dec 7, 2022

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Dec 7, 2022
nikku
nikku reviewed Dec 7, 2022
View reviewed changes
lib/features/popup-menu/PopupMenu.js Outdated
@@ -176,6 +176,8 @@ PopupMenu.prototype.open = function(element, providerId, position, options) {
this._bindAutoClose();

this._render();

this._emit('opened');
Copy link
Member

Choose a reason for hiding this comment

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

Emit when actually opened (i.e. mounted by react component)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

@smbea smbea marked this pull request as draft December 7, 2022 14:32
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 7, 2022
@smbea smbea force-pushed the add-opened-closed-events branch 2 times, most recently from b6612ae to 698def6 Compare December 7, 2022 20:53
@smbea smbea force-pushed the add-opened-closed-events branch 2 times, most recently from d356d52 to ae8af55 Compare December 7, 2022 21:18
@smbea smbea marked this pull request as ready for review December 7, 2022 21:22
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 7, 2022
@smbea smbea requested review from a team, philippfromme, barmac and nikku and removed request for a team December 7, 2022 21:22
nikku
nikku reviewed Dec 7, 2022
View reviewed changes
lib/features/popup-menu/PopupMenu.js Outdated
@@ -122,6 +122,7 @@ PopupMenu.prototype._render = function() {
entries=${ entriesArray }
headerEntries=${ headerEntriesArray }
scale=${ scale }
emitEvent=${ this._emit.bind(this) }
Copy link
Member

Choose a reason for hiding this comment

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

Please expose standard onOpen and onClose handlers.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we emit closed throught the PREACT component? We should be fine with emitting it directly from the popup menu.

Copy link
Contributor Author

@smbea smbea Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In that case it should be fine. But I was trying to keep the same approach for handling of both events

Copy link
Member

@nikku nikku Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Whatever you decide to go for, let's adhere to a common components convention: Props that are passed into a component and are callbacks are called on{NAME}. If you go the generic emit any event route, call the thing onEvent or onLifeCycleEvent.

I'd advocate for a specific handler though. It makes the code easier to maintain, long term.

@smbea smbea requested a review from nikku December 8, 2022 10:01
@smbea smbea merged commit 91e3718 into develop Dec 8, 2022
@smbea smbea deleted the add-opened-closed-events branch December 8, 2022 12:40
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

@philippfromme philippfromme Awaiting requested review from philippfromme philippfromme was automatically assigned from bpmn-io/modeling-dev

@barmac barmac Awaiting requested review from barmac barmac was automatically assigned from bpmn-io/modeling-dev

Assignees

@smbea smbea

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smbea @nikku