chore(Object) removed code for options we dont want to use in _getTransformedDimensions #9620
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
…s/fabric.js into _getTransformedDimension-cleanup
| ) | ||
| ) | ||
| ) { | ||
| if (this.strokeUniform || 'skewX' in options || 'skewY' in options) { |
There was a problem hiding this comment.
there are 4 options and only skewX and skewY are in layoutProperties, so we can check specifically.
|
At company where I work, we actually use this hack to get the transformed dimensions of an object when only some properties (delta) have been changed by another user. So we call the method with the changed methods to calculate what the dims are going to be. Of course we can just define a function that does as before. I'm just wondering what would be the rational of reducing the amount of accepted props while not offering a good alternative. Nobody seems to get benefits here. |
|
it wasn't about benefits but just that his shouldn't be done. I don't need to merge this now if you don't have an alternative, but in general a method that says: You can
All of those are better than this |
From a consumer POV this is actually better than the options you suggested, as it doesn't involve doing mutations (unreliable as you depend on the exact implementation of each underlying method) or creating clones (unreliable as well since some internal states may not be cloned correctly since we're dealing with non-trivial classes, not plain objects). Passing down values is more unconvenient for the maintainer, but far more predictable and reliable for consumer. I wonder if we could borrow some concepts from functional-programming, such as...the Reader Monad? https://dev.to/gcanti/getting-started-with-fp-ts-reader-1ie5. Just kidding. But it could be an idea for instance starting to extract some props under Then having methods using Could potentially also play nicer with reactive states. I've suggested multiple times to @ShaMan123 the need for some sort of higher-level reactive state like https://preactjs.com/guide/v10/signals/, which avoids the need for |
|
i think the solution is very simple, methods work on the instance state, function can work on any passed parameters. So if this kind of predicting size is really useful, it should be come an util, then the method can call the util with its own state, 'this' while the utility itself can be called with any state object you want. But again, this shouldn't really have existed in my opinion, i should have never added scaleX, scaleY, skewX and skewY ages ago, but in that moment supporting skew by mouse seemed so cool to me. I do not know why. I think skewing by mouse is mostly useless. |
|
Is your use case measuring changes to the instance size from changing properties? Is that all? |
True and fabric should limit usage of methods IMO, as it makes overriding harder. You have to ensure the instance state is exactly as expected at each internal method. It's better instead to prepare all the state mutations at once, then call functions passing the state as params.
Yes. I'm just getting the "future" dims passing the changed properties, then I'll use the dims to compute the transform matrix of where the object is gonna be. The transform matrix will then be used as "invariant" thoughout the syncing logic, as it tells where the object MUST be in the end, no matter what's the current transform. |
Description
It happened already that we tried to use _getTransformedDimensions with generic overriding options to work around issues, and then we disagreed on that being a good practice.
This PR reduces the options that are accepted by _getTransformedDimensions to the one we really use.
In Action