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

Remove hover class of hovered element on drop #499

Closed
wants to merge 1 commit into from

Conversation

azeghers
Copy link

Related to bpmn-io/bpmn-js#1366

Acceptance Criteria

  • Corresponds to the concept
  • Corresponds to the design

Definition of Done

@azeghers azeghers requested a review from nikku November 24, 2020 23:29
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Nov 24, 2020
@azeghers
Copy link
Author

@nikku Let me know if this is the right direction and I'll write tests for it 😄

@nikku
Copy link
Member

nikku commented Nov 25, 2020

Please go ahead and write tests regardless of the actual implementation. Test should test the scenario at hand, using our APIs. They are often times more important than the actual implementation (where stuff is fixed and how), as they serve as the spec for the thing fixed.

@nikku
Copy link
Member

nikku commented Nov 25, 2020

Will have a look later today.

*/
eventBus.on('drag.end', function(event) {

canvas.removeMarker(event.hoverGfx, 'hover');
Copy link
Member

Choose a reason for hiding this comment

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

I thought the issue was:

Generally element.hover { A } -> element.out {A} -> element.hover {B} is fired
Firefox sometimes swallows the element.out {A} so we end up with a sequence of element.hover { A } -> element.hover {B} .

My intuition is that we should detect that broken sequence and emit the missing element.out {A}. If that is possible, it would be my preferred option.

Your solution drops the hover class, good. But it still leaves other components hanging that may rely on the proper sequence of hover -> out -> hover events.

@azeghers azeghers closed this Nov 29, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Nov 29, 2020
@azeghers azeghers deleted the 1366-force-remove-hover-on-drop branch November 29, 2020 23:32
@nikku
Copy link
Member

nikku commented Nov 30, 2020

Closed in favor of #503.

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