-
Notifications
You must be signed in to change notification settings - Fork 418
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(popup-menu): allow entries to be draggable #731
Conversation
d19d765
to
a8e456b
Compare
ba664a0
to
44cdd04
Compare
@smbea I've extracted the "complex" test case into an integration test. It should be easier now to verify if the stuff works end-to-end (and how it works). Just Going back to #731 (comment), did you validate the actual feel? Any odd behaviors you found? Do we assume that I do like that |
Is there a concrete reason to not mirror the ContextPad behavior? The end result should be a robust, predictable behavior for the user. |
44cdd04
to
848d805
Compare
@nikku I had another look at it and we can in fact have an almost identical behaviour there - sorry for the back and forth! The one thing I kept different was having the default value "click" for the action. This way we don't introduce a breaking change to the trigger API |
Looks good overall. I left a couple of comments which you can consider to implement but IMO the PR is mergable as is. |
848d805
to
79b4955
Compare
Prevents ugly text selection effect / artifact if entry does not feature a drag action.
79b4955
to
805f203
Compare
UI wise always making the entries draggable and later canceling the default is the more stable behavior; it prevents ugly artifacts (text selection) as seen in the capture below: I've updated the PR to that behavior (which we also have in |
Related to bpmn-io/bpmn-js#1802 -> bpmn-io/bpmn-js#1802 (comment)