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

fix: attach element when used in drag source #20490

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

caalador
Copy link
Contributor

Attach elemnt to dom and move it
outside of the viewport to have it
visible as a drag image.
Image can be used without attaching
to dom. Hidden elements are also
not shown so throw exception
when using one as dragImage.

Fixes #20426

Attach elemnt to dom and move it
outside of the viewport to have it
visible as a drag image.
Image can be used without attaching
to dom. Hidden elements are also
not shown so throw exception
when using one as dragImage.

Fixes #20426
Copy link

github-actions bot commented Nov 18, 2024

Test Results

1 156 files  ± 0  1 156 suites  ±0   1h 35m 15s ⏱️ + 1m 40s
7 498 tests ± 0  7 445 ✅ ± 0  53 💤 ±0  0 ❌ ±0 
7 876 runs  +37  7 813 ✅ +37  63 💤 ±0  0 ❌ ±0 

Results for commit e3a4042. ± Comparison against base commit 2977de2.

♻️ This comment has been updated with latest results.

mshabarov
mshabarov previously approved these changes Nov 18, 2024
Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

I have no strong opinion should the framework add the component by it's own or delegate this to projects. We have agreed that we just document this.
However, with this change, the Flow allows to tune how the component is attached.

Thus, I like this change - to have a default and provide an ability to customise it.

LoggerFactory.getLogger(DragSource.class).info(
"Attaching child to dom in position -100,-100. Consider adding the component manually to not get overlapping components on drag for element.");
getDraggableElement().appendChild(dragElement);
Style style = dragElement.getStyle();
Copy link
Member

Choose a reason for hiding this comment

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

To ensure that appended element does not show up and does not affect DOM unless being dragged, it should probably have display: none by default and change to display: block automatically on drag start event before setting the drag element. And reset back when stopped dragging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work. Tested with the following example:

Div dragBox = new Div("Drag box");
DragSource<Div> dragSource = DragSource.create(dragBox);
dragSource.setDraggable(true);
Button button = new Button("Button");
dragSource.setDragImage(button);
add(dragBox);

It doesn't show anything.

Though it works once I configure it manually:

Div dragBox = new Div("Drag box");
DragSource<Div> dragSource = DragSource.create(dragBox);
dragSource.setDraggable(true);
Button button = new Button("Button");
add(button);
Element element = button.getElement();
Style style = element.getStyle();
style.set("position", "absolute");
style.set("top", "-100px");
style.set("left", "-100px");
// style.set("display", "none"); //also works even if hidden 
dragSource.setDragImage(button);
add(dragBox);

tltv
tltv previously approved these changes Nov 18, 2024
LoggerFactory.getLogger(DragSource.class).info(
"Attaching child to dom in position -100,-100. Consider adding the component manually to not get overlapping components on drag for element.");
getDraggableElement().appendChild(dragElement);
Style style = dragElement.getStyle();
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work. Tested with the following example:

Div dragBox = new Div("Drag box");
DragSource<Div> dragSource = DragSource.create(dragBox);
dragSource.setDraggable(true);
Button button = new Button("Button");
dragSource.setDragImage(button);
add(dragBox);

It doesn't show anything.

Though it works once I configure it manually:

Div dragBox = new Div("Drag box");
DragSource<Div> dragSource = DragSource.create(dragBox);
dragSource.setDraggable(true);
Button button = new Button("Button");
add(button);
Element element = button.getElement();
Style style = element.getStyle();
style.set("position", "absolute");
style.set("top", "-100px");
style.set("left", "-100px");
// style.set("display", "none"); //also works even if hidden 
dragSource.setDragImage(button);
add(dragBox);

Log attach as debug now that the

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
Copy link

sonarcloud bot commented Nov 19, 2024

@mshabarov
Copy link
Contributor

Added a workaround to docs, while this is being investigated. vaadin/docs#3936

@mshabarov
Copy link
Contributor

I tried doing the same trick using a plain JavaScript and it works as expected for me, the styles are similar and the drag image component is a child of drag source component. I'll go checking what's wrong in Flow because a browser is likely out of the loop here.

setdragimage

@mshabarov
Copy link
Contributor

It was my fault, I had

.v-dragged {
  opacity: 0;
}

leftover css in my test project that ofc made things worse.

@mshabarov mshabarov merged commit 69e47a5 into main Nov 21, 2024
26 checks passed
@mshabarov mshabarov deleted the issues/20426-drag-element branch November 21, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for invisible/detached components for setDragImage
4 participants