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

add transform to potential conflict attributes #130

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tiberiuzuld
Copy link

Hi @timruffles,
Have a use case where the items I want to drag already have transform on they're style, making the transform empty string fixes my issue of having the image outside the view port.

Hi @timruffles,
Have a use case where the items I want to drag already have transform on they're style, making the transform empty string fixes my issue of having the image outside the view port.
@reppners
Copy link
Collaborator

Welcome and thanks for your contribution :)

Can you provide a demo of your use case? I'm afraid this change could break existing polyfill users that might rely on the transform style being present on some nested node of a draggable element.

Would it be enough to clear the transform of the drag image element instead of clearing it on all child nodes?

Meanwhile you can apply your changes with a custom dragImageSetup function, see https://github.com/timruffles/mobile-drag-drop#custom-drag-image-setup-function

It's available starting v2.3.0-rc.0

@tiberiuzuld
Copy link
Author

tiberiuzuld commented Apr 22, 2018

Yes it will be enough to set it on the drag image only, now I saw that the function goes recursive through all children.
Ok will make a change to add the empty transform in the createDragImage function https://github.com/tiberiuzuld/mobile-drag-drop/blob/b3df77a12b1f102be81f7265c63c8b64bceb6cf9/src/internal/dom-utils.ts#L83

Will be that ok?
Edit: hmm not ok because the copy of computed styles in prepareNodeCopyAsDragImage will put it in.

And yes was also thinking in making my own function for createDragImage but then need to implement function prepareNodeCopyAsDragImage.

On the library I work, I position elements with transform.
Demo here: https://tiberiuzuld.github.io/angular-gridster2/
Working on a branch now to enable native drag and drop between 2 dashboards: https://github.com/tiberiuzuld/angular-gridster2/tree/drag

@tiberiuzuld
Copy link
Author

Ok added the empty transform only for the first node.
Will this be ok ?

@reppners
Copy link
Collaborator

I'm not capable of reviewing your PR right away because of little to no free time. Planning to get to it done next weekend but can't make a promise here.

The change needs thorough review because there is some transform related regex stuff in the polyfill where I need to make sure we're not breaking others - will need to set up some demo content with extensive transform usage to make sure all is working as expected.

@tiberiuzuld
Copy link
Author

No problem, have little free time also.
Was also thinking if it would be better to add a div wrapper around the element dragged?
It will not fix the issue if the dragged node has left, top, bottom, right or transform.
But if we set position: static to the dragged element inside the div wrapper then element will ignore position styles.

@reppners
Copy link
Collaborator

Was also thinking if it would be better to add a div wrapper around the element dragged?
It will not fix the issue if the dragged node has left, top, bottom, right or transform.
But if we set position: static to the dragged element inside the div wrapper then element will ignore position styles.

The more I look at the code involved the more I think your suggestion is the cleaner approach. It would also allow to remove the current regex transform fiddling which is fragile because it does not cover all the variants that are possible on the transform property.

Changing this part of the code has a high risk of breaking someone that might rely on current behaviour so this likely requires a major version bump.

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