-
Notifications
You must be signed in to change notification settings - Fork 949
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
add drag and drop based on @btel's work #2720
base: main
Are you sure you want to change the base?
Conversation
I thought this was great, thank you! I added a comment inline. |
This looks good to me :) |
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.
LGTM!
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"`DraggableLabel` is a label that can be dragged and dropped to other fields." |
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.
It looks like DraggableLabel
can be removed from the example notebook?
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.
Never mind that, I thought it was initially imported from ipywidgets
(https://github.com/jupyter-widgets/ipywidgets/pull/2363/files for reference).
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.
Indeed, the initial implementation had a DraggableLabel
, but we removed it in favor of the more general DraggableBox
.
I am in favor of merging this. Leaving it open for a little bit more so that @wolfv gets to fix the small inline comments. |
|
||
this.pWidget.addClass('jupyter-widgets'); | ||
this.pWidget.addClass('widget-container'); | ||
this.pWidget.addClass('widget-draggable-box'); |
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.
Is this class needed? It doesn't seem to be used.
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.
I think I just tried to follow conventions:
ipywidgets/packages/controls/src/widget_box.ts
Lines 83 to 85 in 7cb20cd
this.pWidget.addClass('jupyter-widgets'); | |
this.pWidget.addClass('widget-container'); | |
this.pWidget.addClass('widget-box'); |
someone who wants to style the drag drop widgets might want to add CSS to this?
On the other hand, the two leaf classes should probably add another class as well.
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.
someone who wants to style the drag drop widgets might want to add CSS to this?
That could indeed be useful.
For the case of the widget_box, there is actually some default css:
ipywidgets/packages/controls/css/widgets-base.css
Lines 98 to 103 in 7cb20cd
.widget-box { | |
box-sizing: border-box; | |
display: flex; | |
margin: 0; | |
overflow: auto; | |
} |
One thing that I am afraid of with these additional events is that depending on the latency, things might get out of order on slow connections ... E.g. if we had a drag_start event and allow to modify drag_data, we somehow need to guarantee that the new drag_data is set before the widget is dropped etc. |
Yes it sounds like there could be some race conditions indeed. If we want to still be able to add a visual feedback, an alternative could be to keep these events on the frontend and provide a way to customize the hover effect (for example by setting |
Agree, adding a CSS class on hover would be nice! I was thinking to keep this PR minimal to make sure it gets in, then we can build on top :) |
Impressive! |
This looks so promising, can't wait to use it in my projects ;) |
That's insane! Thanks @jtpio. I didn't even know that you can create a window with a widget view. It's only a step away from an interactive builder for voila dashboards ;-) |
I rebased & regenerated the spec :) |
Tried to fix it :) |
Looks good 👍 |
|
||
if data.get('application/vnd.jupyter.widget-view+json'): | ||
widget_mime = json.loads(data['application/vnd.jupyter.widget-view+json']) | ||
data['widget'] = widget_serialization['from_json']('IPY_MODEL_' + widget_mime['model_id']) |
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.
I'm thinking about this. Why is this method exposed as an API method? If the user is meant to use it from the kernel side, should we not handle the deserialization of the widget reference before calling this? This way, any kernel use could simply pass a widget reference as widget
, and not have to worry about the model_id
.
This is a really nice addition! Is the on_drop callback extensible to handle more than widgets in the future, e.g. dragging an out-of-browser file into a widget? |
When will this drag and drop functionality be available? I can't wait to try this out. |
Any ETA on this? This would be very useful for me. |
Wondering about the same. such a feature would be very useful for building dynamic dashboards. |
I rebased this PR and fixed some minor issues. gf_drag_drop. I think this is a really cool feature. Shall I create another PR? |
yes, please! :) |
No description provided.