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

[POC] Allow string default position #271

Closed
wants to merge 3 commits into from

Conversation

eric-burel
Copy link

Hi,
Since react-draggable relies on transform, it can sometimes be difficult to create components that rely themselves on transform, since transformations will be overidden by react-draggable.

This PR especially solves #223. Indeed, the only way to center a floating div on the screen is to use a transform(-50%, -50%).

So, I added the possibility to provide the defaultPosition as a string.

Cool stuffs:

  • we can provide a relative defaultPosition, this is good for centering
  • it does the job

Bad stuffs:

  • it breaks automatic tests and sorry, I lack the time to update them (the live example is working though)
  • it relies on calc, I am not sure if this is good / cross-browser / IE compatible
  • it add computations in the transform generation, which reduce performance (but is it significant ?), instead of using the defaultPosition only during the component construction.
  • it is not optimized, I put calc everytime even if all values are string

@STRML
Copy link
Collaborator

STRML commented Aug 21, 2017

Hi - apologies that master is now conflicting, could you make the following changes:

  1. Don't commit dist/, and
  2. Port your changes from the old .es6 files (this extension had issues with some tools like flow-upgrade) over to .js.

Thanks for your contribution! 👍

@tnrich
Copy link
Contributor

tnrich commented Sep 1, 2018

@eric-burel can you please finish this PR? I would also like to see this work!

@STRML please please make this PR work

@tnrich
Copy link
Contributor

tnrich commented Sep 1, 2018

I made #361 which fixes @STRML's above comments.

@eric-burel
Copy link
Author

Thanks @tnrich, I close this PR since you opened a new one.

@eric-burel eric-burel closed this Sep 3, 2018
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.

None yet

3 participants