defaultPosition now allows strings by tnrich · Pull Request #361 · react-grid-layout/react-draggable

tnrich · 2018-09-01T19:33:31Z

This is a copy of #271

but with the changes @STRML requested

… createSVGTransform to use calc with defaultPosition, adding a test for the new functionality

tnrich · 2018-09-01T21:51:49Z

@STRML @eric-burel I fixed a couple issues in the commit I just made. Linting and tests now pass and I added a test for the new use case.

tnrich · 2018-09-01T21:53:35Z

@bokuweb when this PR gets merged this issue bokuweb/react-rnd#437 will no longer be relevant. I should just be able to pass "50%" for both x and y defaultPositions

tnrich · 2018-09-01T22:01:57Z

@STRML any idea why this would be failing only on IE?

That's the new spec I added:
IE 11.0.0 (Windows 8.1 0.0.0) react-draggable props should render with defaultPosition set as string transform and handle subsequent translate() for DOM nodes FAILED
AssertionError: # specs\draggable.spec.jsx:255

  assert(style.indexOf('translate(calc(10% + 100px), calc(10% + 100px));') >= 0)
         |     |                                                           |    
         |     -1                                                          false
         "touch-action: none;"                                                  
  
   at fail (specs/main.js:27336:3)
   at ok (specs/main.js:27356:15)
   at Decorator.prototype._callFunc (specs/main.js:30707:9)
   at Decorator.prototype.concreteAssert (specs/main.js:30700:5)
   at decoratedAssert (specs/main.js:31011:13)
   at powerAssert (specs/main.js:4743:13)
   at Anonymous function (specs/main.js:6344:7)

tnrich · 2018-09-01T22:04:57Z

hmm looks like it is related to this:

I'm not sure if there is some way around this..?

eric-burel · 2018-09-03T07:22:29Z

Hi @tnrich thanks a lot for your work on this. I only used react-draggable once last year, I already work on other open source projects and I am not a CSS export so I can not help much :/
I guess the solution is to detect that we are using IE11 and using JS to handle position in this case.

…tes instead of a calc function do to IE incompatibility; updating spec to reflect changes

tnrich · 2018-09-03T19:13:01Z

Okay found a solution based on this stackoverflow answer: https://stackoverflow.com/questions/21142923/ie-10-11-css-transitions-with-calc-do-not-work

this commit fixes it and tests should now pass :)
cf6979a

updating createCSSTransform and createSVGTransform to use two translates instead of a calc function do to IE incompatibility; updating spec to reflect changes

tnrich · 2018-09-05T20:21:45Z

@STRML bump :)

tnrich · 2018-09-10T16:05:58Z

@mzabriskie @STRML any thoughts?

tnrich · 2018-09-17T21:33:44Z

bumping. @STRML is this project still active?

tnrich · 2018-10-08T07:13:11Z

@STRML bump

STRML · 2018-11-10T13:05:46Z

 }

-export function createCSSTransform({x, y}: {x: number, y: number}): Object {
+export function createCSSTransform({x, y}: {x: number, y: number}, defaultPosition: {x: number | string, y: number | string }): Object {


You create a DefaultControlPosition type above but then don't use it here.

I now use it here

STRML · 2018-11-10T13:09:03Z

+  const translation = (defaultPosition && defaultPosition.x !== 0 && defaultPosition.y !== 0)
+  ? `translate(${defaultX}, ${defaultY}) translate(${x}px,${y}px)`
+  : 'translate(' + x + 'px,' + y + 'px)';
  // Replace unitless items with px


This comment no longer makes sense with the changes above.

Removed the comment

STRML · 2018-11-10T13:31:34Z


-export function createCSSTransform({x, y}: {x: number, y: number}): Object {
+export function createCSSTransform({x, y}: {x: number, y: number}, defaultPosition: {x: number | string, y: number | string }): Object {
+  const defaultX = (typeof defaultPosition.x === 'string') ? defaultPosition.x : defaultPosition.x + 'px';


This is really difficult to read. You create these two consts, defaultX and defaultY, but they're only actually used if and only if a defaultPosition is actually present.

You then use that just to prepend what is being returned as a second translate.

This is a lot of logic in what was previously a simple function and should be refactored such that the caller prepends the default translate if needed.

Reworked this to hopefully be more clear

STRML · 2018-11-10T13:31:42Z

+  const defaultY = (typeof defaultPosition.y === 'string') ? defaultPosition.y : defaultPosition.y + 'px';
+  const translation = (defaultPosition && defaultPosition.x !== 0 && defaultPosition.y !== 0)
+  ? `translate(${defaultX}, ${defaultY}) translate(${x}px,${y}px)`
+  : 'translate(' + x + 'px,' + y + 'px)';


Why no template string here?

Added template strings everywhere I could

STRML · 2018-11-10T13:32:14Z

+  const defaultX = (typeof defaultPosition.x === 'string') ? defaultPosition.x : defaultPosition.x + 'px';
+  const defaultY = (typeof defaultPosition.y === 'string') ? defaultPosition.y : defaultPosition.y + 'px';
+  const translation = (defaultPosition && defaultPosition.x !== 0 && defaultPosition.y !== 0)
+  ? `translate(${defaultX}, ${defaultY}) translate(${x}px,${y}px)`


This is wrong, svg transforms shouldn't have px.

This should no longer have px in it

…ving readability

tnrich · 2018-11-27T22:09:21Z

@STRML let me know if there is anything else I can change to get this PR accepted and merged? Thanks!

pavog · 2019-02-26T12:08:39Z

Any updates on this? @tnrich @STRML

tnrich · 2019-02-26T16:58:03Z

@pavog I believe I made all the changes @STRML requested the first time around. We're just waiting on him?

pavog · 2019-02-27T09:19:17Z

@tnrich Yep, I believe that too. @STRML is still active (see #389) in this repository but seems to not want to review your changes again.

pavog · 2019-02-27T09:33:49Z

@STRML Thank you very much 👍

This reverts commit aa0a6b4.

)" This reverts commit aa0a6b4.

eric-burel · 2019-03-15T08:51:06Z

Awesome, thanks all for finishing this PR and merging !

tnrich · 2019-03-15T16:11:14Z

@eric-burel @STRML had to revert the changes due to unexpected breakages. I've made a new PR that hopefully solves those issues by introducing a new property initialPosition

#393

defaultPosition now allows strings

2318ec9

tnrich mentioned this pull request Sep 1, 2018

[POC] Allow string default position #271

Closed

fixing flow linting by adding new DefaultControlPosition type, fixing…

51652d6

… createSVGTransform to use calc with defaultPosition, adding a test for the new functionality

updating createCSSTransform and createSVGTransform to use two transla…

cf6979a

…tes instead of a calc function do to IE incompatibility; updating spec to reflect changes

tnrich mentioned this pull request Oct 23, 2018

Is this repo still being maintained? #369

Closed

STRML requested changes Nov 10, 2018

View reviewed changes

using DefaultControlPosition; fixing svg transform units issue; impro…

d5f0812

…ving readability

types(domFns): use ControlPosition

a29db69

STRML approved these changes Feb 27, 2019

View reviewed changes

STRML merged commit aa0a6b4 into react-grid-layout:master Feb 27, 2019

STRML mentioned this pull request Mar 1, 2019

3.2.0 Breaking Change: Drag Callbacks #391

Closed

STRML added a commit that referenced this pull request Mar 1, 2019

Revert "feat(defaultPosition): now allows strings (#361)"

8b1449c

This reverts commit aa0a6b4.

tnrich added a commit to tnrich/react-draggable that referenced this pull request Mar 1, 2019

Revert "feat(defaultPosition): now allows strings (react-grid-layout#361

1469b7b

)" This reverts commit aa0a6b4.

renovate Bot mentioned this pull request Jun 6, 2021

Update dependency react-draggable to v4 aestetype/react-fixed-grid#8

Open

1 task

This was referenced Jul 27, 2021

fix(deps): update dependency react-draggable to v3.3.2 - abandoned scratchfoundation/scratch-gui#7420

Closed

fix(deps): update dependency react-draggable to v4 scratchfoundation/scratch-gui#7453

Open

Uh oh!

Conversation

tnrich commented Sep 1, 2018

Uh oh!

tnrich commented Sep 1, 2018

Uh oh!

tnrich commented Sep 1, 2018

Uh oh!

tnrich commented Sep 1, 2018

Uh oh!

tnrich commented Sep 1, 2018

Uh oh!

eric-burel commented Sep 3, 2018

Uh oh!

tnrich commented Sep 3, 2018

Uh oh!

tnrich commented Sep 5, 2018

Uh oh!

tnrich commented Sep 10, 2018

Uh oh!

tnrich commented Sep 17, 2018

Uh oh!

tnrich commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnrich commented Nov 27, 2018

Uh oh!

pavog commented Feb 26, 2019

Uh oh!

tnrich commented Feb 26, 2019

Uh oh!

pavog commented Feb 27, 2019

Uh oh!

pavog commented Feb 27, 2019

Uh oh!

eric-burel commented Mar 15, 2019

Uh oh!

tnrich commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants