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

chore(TS): utils #8230

Merged
merged 37 commits into from Sep 3, 2022
Merged

chore(TS): utils #8230

merged 37 commits into from Sep 3, 2022

Conversation

asturur
Copy link
Member

@asturur asturur commented Sep 1, 2022

  • removed getById in favor of normal browser js
  • removed toArray in favor of Array.from
  • removed addClass in favor of classList.add
  • moved the rotation point logic all in the point class
  • removed makeElement
  • removed setImageSmoothing

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Coverage after merging see-where-i-broke into master will be

81.01%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
dist
   fabric.js79.33%75.46%84.66%81.01%10006, 10025–10027, 10029–10030, 10092, 10146, 10161, 10217, 10253–10254, 10260, 10264–10265, 10280, 10314, 10345–10346, 10370, 10378, 10465, 10497–10498, 10573–10574, 10577, 10582, 10604, 10604, 10604, 10604, 10604, 10604, 10604–10605, 10607–10608, 10608, 10608–10609, 10614, 10616, 10622, 10622, 10622, 10622, 10622–10623, 10625–10629, 10629, 10629–10631, 10633–10634, 10643, 10654, 10665, 10676, 10686–10689, 10697–10698, 10698, 10698–10699, 10701–10702, 10709, 10713, 1085, 11310, 11315, 11357–11359, 11359, 11359, 11359, 11359, 11359–11360, 11380, 11385–11386, 11426, 11426, 11426, 11426, 11454, 11454, 11454, 11456, 116, 1160, 11604–11605, 11616, 1162–1163, 1165–1166, 1173, 1177, 11773–11774, 11781, 11824, 11831, 11831, 11840–11841, 1185, 11852, 11892, 119, 11900, 11919–11921, 11948–11949, 11952, 11952, 11969–11971, 120, 12074, 121, 12159–12160, 12162–12163, 12170, 12194, 1221, 1223, 1223, 1223, 1223, 1223–1224, 12259–12260, 12264, 12344, 12355, 12360, 12397, 12498, 12498, 12498, 12522–12523, 12631, 12634, 12696, 12702, 12709, 12716, 12722, 12728, 12735, 12742, 12748–12749, 12749, 12749, 12764–12765, 12773, 12782, 12782, 128, 12807–12810, 12849, 12892–12893, 129, 129, 129, 129, 129, 129, 129, 12933–12934, 130, 13067–13068, 131, 13213, 133, 13305, 13368, 13371, 13424–13425, 13425, 13425, 13428, 13443, 13457, 13469–13470, 13472, 13484–13485, 13487, 13502, 13517–13518, 13520–13521, 13523–13524, 13534, 13564, 13564, 13564, 13564, 13564, 13564, 13564, 13564, 13589, 13591, 13591–13593, 13646–13648, 13671, 13679, 13685–13686, 13727, 13790–13791, 13825, 13846–13847, 1390, 1390, 13906, 13906, 13906, 13906, 13906, 13911–13919, 1392, 13961, 14087–14088, 14124, 14133, 14154, 14154, 14154–14155, 14155, 14155, 14155, 14155–14156, 14162–14164, 14167–14168, 14181, 14181, 14181–14182, 14182, 14182, 14182, 14182–14183, 14189–14191, 14194–14195, 14208–14209, 14283, 14332, 14336, 1442, 14461, 14491–14492, 14492, 14492–14493, 14493, 14493–14494, 14496, 14496, 14496–14497, 14500, 14507, 14592, 14705, 14754, 14824, 14828, 14898, 14955–14956, 14970, 15020–15021, 15023–15024, 15116, 15176, 15179, 15254, 15257, 15272–15273, 15282, 15324, 15328, 15353–15354, 15388, 15392, 1562, 15658, 15662, 15826, 15829, 15836–15837, 15837, 15837–15839, 15841, 15841, 15841–15843, 15843, 15843–15845, 15935, 15940–15942, 15970–15971, 16038, 16041, 16041, 16041–16042, 16044–16045, 16045, 16045–16046, 16046, 16046, 16048–16049, 16051, 16054, 16081, 16081–16083, 16132, 16132, 16167, 16202, 16202, 16202, 16205–16206, 16208, 16208, 16212, 16215, 16218, 16220–16221, 1623, 16230, 16235, 16235–16237, 16237, 16248–16249, 16258, 16258, 16258, 16258, 16258–16259, 16259, 16259–16261, 16261–16262, 16262, 16279–16280, 16280, 16280, 16302–16303, 16303, 16303, 16303, 16303, 16303, 16315, 16315, 16318–16322, 16322, 16361, 16400, 16432, 16435–16438, 16448, 16461, 16476, 16493, 16502, 16506, 16558, 16560–16562, 16576, 16578–16579, 16595, 16647, 16670, 16803, 16866–16868, 16868–16869, 16869, 16897, 16942–16945, 16952, 16954–16955, 16970–16972, 16982, 16982, 16982, 17045, 17057, 17057, 17104–17105, 17123–17124, 17145, 17186, 17186–17187, 17204–17207, 17207, 17207–17208, 17210, 17210, 17210–17211, 17213–17214, 17214, 17214–17215, 17217, 17217, 17217–17218, 17220–17221, 17225–17226, 17273, 17317, 17386, 17391, 17415–17416, 17425–17429, 17441–17444, 17449, 17458–17459, 17461, 17470, 17470, 17470, 17470, 17470–17471, 17473–17474, 17490–17491, 17493–17494, 17501–17504, 17507, 17510, 17512–17513, 17513, 17513, 17513, 17513, 17513, 17513–17514, 17516, 17518–17519, 17519, 17519–17522, 17524, 17531–17539, 17539, 17539–17541, 17544, 17552–17555, 17561–17562, 17562, 17562–17563, 17565, 17565, 17565–17566, 17568, 17570–17571, 17586, 17588, 17588, 17588–17589, 17591–17592, 17592–17593, 17593, 17599, 17599, 1760, 17601, 17601–17602, 17602, 1761, 17611–17613, 17613, 17613–17621, 17627, 17627, 17627–17629, 17631, 17637–17638, 17652–17658, 17658, 17658–17659, 17662, 17664, 17676, 17676, 17676–17677, 17680–17682, 17692, 17692, 17692–17694, 17706, 17706, 17706–17707, 17709–17710, 17710, 17710–17711, 17713–17714, 17714, 17714–17717, 17717, 17717–17718, 17720, 17720, 17720–17721, 17724–17725, 17728, 17730–17731, 17731, 17731, 17731, 17731–17733, 17747–17749, 17751–17752, 17763, 17765, 17767–17770, 17827, 17893,

src/canvas.class.ts Outdated Show resolved Hide resolved
ShaMan123 and others added 10 commits September 1, 2022 19:21
commit 51b93a8
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Sep 1 19:17:52 2022 +0300

    fix tests!

commit 672ff98
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Sep 1 19:17:42 2022 +0300

    Update image.class.ts

commit edab4e1
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Thu Sep 1 19:17:39 2022 +0300

    Update canvas.class.ts

commit 0e23df8
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Thu Sep 1 16:15:39 2022 +0200

    ok?

commit 09eb1b5
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Thu Sep 1 16:03:55 2022 +0200

    addClass removed for classList.add)
// but then we remove the lower-canvas specific className
upperCanvasEl.classList.remove('lower-canvas');
// we add the specific upper-canvas class
upperCanvasEl.classList.add('upper-canvas');
Copy link
Contributor

Choose a reason for hiding this comment

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

much better

limitDimsByArea(ar: number) {
const { perfLimitSizeTotal } = config;
const roughWidth = Math.sqrt(perfLimitSizeTotal * ar);
// we are not returning a point on purpose, to avoid circular dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

VERY STRANGE
And shouldn't happen...
HACKY

Copy link
Member Author

Choose a reason for hiding this comment

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

no is not hacky and not everything should be a point.
Before was x and y but wasn't a point.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for the circular dep cycle is just because of the assignment fabric.Point that will be removed anyways.
If you assign it in index it will not happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you agree this is out of pattern?

Copy link
Member Author

Choose a reason for hiding this comment

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

is a function about caching, i don't think returning an array to return 2 values is better or worse than returnin an object and calling it a point. I just really need the 2 values out of there.

});
const container = fabric.document.createElement('div');
container.classList.add(this.containerClass);
this.wrapperEl = fabric.util.wrapElement(this.lowerCanvasEl, container);
Copy link
Contributor

Choose a reason for hiding this comment

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

drop wrapElement from utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

wrapElement probably don't belong in utils exported object indeed. will be just a function imported here

Copy link
Contributor

Choose a reason for hiding this comment

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

it is used only here across fabric, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes i think only here to add a div around the canvas

@@ -1,8 +1,9 @@
import { TMat2D } from "./typedefs";

export { version as VERSION } from '../package.json';
export const noop = () => {};
export function noop() {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between const = () => {} and function(){}?
Or just styling?
I don't mind

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a difference that a fat arrow can't be bound and can't be used as a class member.
So since noop is used in createClass as an empty method, just to be sure i made it a function

@@ -378,4 +386,6 @@ export class Point {
}
}

const originZero = new Point(0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to reuse this? That is why you don't inline it in the function signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think is more readable defined as a const rather than in the function definition.
Does it get defined every function run if is in the function signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

good question
no idea

* Backwards easing out
* @memberOf fabric.util.ease
*/
export const easeOutBack = (t, b, c, d, s = 1.70158) => c * ((t = t / d - 1) * t * ((s + 1) * t + s) + 1) + b;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this number a lot. Maybe define it as const at top of file?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes there are a bunch of numbers. i wanted to define them, then i realized i didn't know how to call them and moved on for now

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally get that

*/
export const easeOutBounce = (t, b, c, d) => {
if ((t /= d) < (1 / 2.75)) {
return c * (7.5625 * t * t) + b;
Copy link
Contributor

Choose a reason for hiding this comment

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

more magic numbers that can/should be defined at top of file?

var docElem,
doc = element && element.ownerDocument,
box = { left: 0, top: 0 },
return { left, top };
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a point as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure, this is style related, it does not need to be transformed or anything else.

function getNodeCanvas(element) {
var impl = fabric.jsdomImplForWrapper(element);
return impl._canvas || impl._image;
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Point?

Copy link
Contributor

Choose a reason for hiding this comment

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

better off for rtl support as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think those are just for calculating the canvas position for some sort of padding/border issue.
I m not sure we ever need a different measurement.

I also think modern browsers solves this for us giving us an extra pair of mouse event coordinates relative to the canvas element that would be exactly what we need

Copy link
Member Author

Choose a reason for hiding this comment

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

yes i think that i should look into moving mouse events with offsetX and offsetY and trash all of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

i ll open a task

* @param {HTMLElement} element Element to make selectable
* @return {HTMLElement} Element that was passed in
*/
export function makeElementSelectable(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

merge makeElementSelectable and makeElementUnselectable to one method with a flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

not the kind of refactors or changes i m looking to now.
There is still a lot to do, those are really minors utils, i can merge them yes, if i manage to make everything else work.

@asturur
Copy link
Member Author

asturur commented Sep 2, 2022

This is the list of utils from the utils folder, with some ideas of what to continue to expose and what to expose somewhere else as deprecated ( and to become internals )

fabric.util = {
  cos,
  sin,
  rotateVector,
  createVector,
  calcAngleBetweenVectors,
  getHatVector,
  getBisector,
  degreesToRadians,
  radiansToDegrees,
  rotatePoint,
  getRandomInt, // hide
  removeFromArray, // hide
  projectStrokeOnPoints,
  transformPoint,
  invertTransform,
  composeMatrix,
  qrDecompose,
  calcDimensionsMatrix,
  calcRotateMatrix,
  multiplyTransformMatrices,
  stylesFromArray,
  stylesToArray,
  hasStyleChanged, // hide
  object: {
    clone, // hide
    extend, // hide
  },
  createCanvasElement,
  createImage,
  copyCanvasElement,
  toDataURL,
  toFixed,
  matrixToSVG, // hide
  parsePreserveAspectRatioAttribute, // hide
  groupSVGElements, // hide
  parseUnit, 
  getSvgAttributes,
  findScaleToFit,
  findScaleToCover,
  capValue, // hide
  saveObjectTransform,
  resetObjectTransform,
  addTransformToObject,
  applyTransformToObject,
  removeTransformFromObject,
  makeBoundingBoxFromPoints,
  sendPointToPlane,
  transformPointRelativeToCanvas, // this has to be renamed imho
  sendObjectToPlane,
  string: {
    camelize, // hide
    capitalize, // hide
    escapeXml, // hide
    graphemeSplit, // this needs a setter and getter to be overridden
  },
  getKlass, // hide
  loadImage,
  enlivenObjects,
  enlivenObjectEnlivables,
  array: {
    min, // hide
    max, // hide
  },
  pick, // hide
  joinPath,
  parsePath,
  makePathSimpler,
  getSmoothPathFromPoints,
  getPathSegmentsInfo,
  getBoundsOfCurve,
  getPointOnPath,
  transformPath,
  getRegularPolygonPath,
  request, // hide
  setStyle,
  isTouchEvent,
  getPointer,
  removeListener, // hide
  addListener, // hide
  wrapElement, // hide
  getScrollLeftTop, // hide
  getElementOffset, // hide
  getNodeCanvas,
  cleanUpJsdomNode,
  makeElementUnselectable, // hide
  makeElementSelectable, // hide
  isTransparent,
  sizeAfterTransform,
  mergeClipPaths,
  ease,
  animateColor,
  animate,
  requestAnimFrame,
  cancelAnimFrame,
};

some i m not sure like getBiSector, getHatVector, if those are just internals for calculation or have some utility for projects.

@ShaMan123
Copy link
Contributor

transformPointRelativeToCanvas has to be renamed for sure and needs work I think, doesn't account for canvas offset.
Should redo it based on the logic in canvas, extract it from there...

matrixToSVG and parsePreserveAspectRatioAttribute are exposed in parser right? So a dev can use them if needed to create custom parsing, right?

Looks good to me

@asturur
Copy link
Member Author

asturur commented Sep 2, 2022

transformPointRelativeToCanvas has to be renamed for sure and needs work I think, doesn't account for canvas offset.
Should redo it based on the logic in canvas, extract it from there...

Ok so i m not sure i understaood that function. to me it just seems it converts from viewportCoordinates to htmlElement coordinates. What does it do on top of that?

@asturur
Copy link
Member Author

asturur commented Sep 2, 2022

The takeaway from this PRs are:

There are more improvements that can be made to this code probably, but the intention of this PR was to move it to new import/exports and remove the old es5 code hacks.

If there are no objections i would like to merge this tomorrow and move to the class/function discussion

@ShaMan123
Copy link
Contributor

I can review later
Or merge and we move forward

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I still disapprove of limitDimsByArea return type.
Think of a dev subclassing caching.
But let's move on.

General To Do

  • cache can become standalone exports instead of a class once we make everything a map
  • add onStart to animate - since it complements delay
  • sendPointRelativeToCanvas - rename to sendPointToWindow and sendPointToCanvas? should utilize getPointer code from canvas.

})(start);
};

setTimeout(() => requestAnimFrame(runner), delay);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this change since it makes all animations start in delay even if delay is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

is 0 by default is not undefined !
i thought the code semplification was worth a setTimeout 0

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice that.
I am not sure is worth...

Copy link
Member Author

Choose a reason for hiding this comment

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

this change can be reverted to:

if (delay > 0 ) {
  setTimeout(() => requestAnimFrame(runner), delay);
} else {
  requestAnimFrame(runner)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

undefined should be not setTimeout
0 we need to decide if behaves as undefined or as setTimeout 0 does

Copy link
Member Author

Choose a reason for hiding this comment

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

i removed delay undefined. delay is a number, it will be 0 or a number bigger than 0.

Copy link
Contributor

@ShaMan123 ShaMan123 Sep 3, 2022

Choose a reason for hiding this comment

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

Great, even just if(delay)

Copy link
Contributor

@ShaMan123 ShaMan123 Sep 3, 2022

Choose a reason for hiding this comment

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

I get why >0.

Copy link
Member Author

Choose a reason for hiding this comment

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

this has been changed back to the more verbose version

* @param {Function} callback Callback to invoke
* @param {DOMElement} element optional Element to associate with animation
*/
export function requestAnimFrame(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't args just [callback]?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe, but since the function is just a passtrough for some reason, i keep it generic

return _requestAnimFrame.apply(fabric.window, args);
}

export function cancelAnimFrame(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

b = c1;
}
// `b` becomes `a`'s clip path so we transform `b` to `a` coordinate plane
applyTransformToObject(
Copy link
Contributor

@ShaMan123 ShaMan123 Sep 3, 2022

Choose a reason for hiding this comment

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

It must be.
That is exactly what the function does.
We return to it later

@ShaMan123
Copy link
Contributor

ShaMan123 commented Sep 3, 2022

transformPointRelativeToCanvas has to be renamed for sure and needs work I think, doesn't account for canvas offset.
Should redo it based on the logic in canvas, extract it from there...

Ok so i m not sure i understaood that function. to me it just seems it converts from viewportCoordinates to htmlElement coordinates. What does it do on top of that?

It should do that but it doesn't account for canvas offset and it should

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

@ShaMan123 i reverted your commit.

this is sendObjectToPlane:

  const t = multiplyTransformMatrices(invertTransform(to), from);
  applyTransformToObject(
    object,
    multiplyTransformMatrices(t, object.calcOwnMatrix())
  );

it has obect.calcOwnMatrix as extra and 2 matrices to and from. 2 uses of multiplyTransformMatrices.

the clipPath function has a multiplication less.

  applyTransformToObject(
    b,
    multiplyTransformMatrices(
      invertTransform(a.calcTransformMatrix()),
      b.calcTransformMatrix()
    )
  );

I told you i have tried it and didn't work.
If those are identical and i can't get it, point it out

@ShaMan123
Copy link
Contributor

ShaMan123 commented Sep 3, 2022

That is why the following is correct.

- sendObjectToPlane(b, b.calcTransformMatrix, a.calcTransformMatrix())
+ sendObjectToPlane(b, b.group?.calcTransformMatrix, a.calcTransformMatrix())

Taking b's containing plane matrix and multiplying with b's own matrix gives us b.calcTransformMatrix()

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

  • sendPointRelativeToCanvas - rename to sendPointToWindow and sendPointToCanvas? should utilize getPointer code from canvas.

I m not sure i get it. There are no window coordinates ever, we either use design coordinate ( your fabricjs canvas creation, or the htmlCanvas element coordinates ( the distance between the point and the left top HTMLCanvas corner ))

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

That is why the following is correct.

- sendObjectToPlane(b, b.calcTransformMatrix(), a.calcTransformMatrix())
+ sendObjectToPlane(b, b.group?.calcTransformMatrix(), a.calcTransformMatrix())

Taking b's containing plane matrix and multiplying with b's own matrix gives us b.calcTransformMatrix()

I don't think so, i think you are inverting a multiplcation order.
If there is no group, you send null ( let's say is conditionally an IMatrix ).
In the case the group exist you are going to multiply forst for the group matrix and then for the object one,
in the case as it is today, you are multiplying for the ( OBJECT x CONTAINER ).

I ll write a failing unit test, and then you'll owe me a cocacola first time we ever meet.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Sep 3, 2022

We are on! When will we meet and where?

@ShaMan123
Copy link
Contributor

Let's agree that if we meet in Israel it will something much tastier. Accepted?

@ShaMan123
Copy link
Contributor

I m not sure i get it. There are no window coordinates ever, we either use design coordinate ( your fabricjs canvas creation, or the htmlCanvas element coordinates ( the distance between the point and the left top HTMLCanvas corner ))

I wrote sendPointRelativeToCanvas so a dev could interact with HTML objects in the dom from a nested object for example. We can trash it completely and I'll redo it later.

@ShaMan123
Copy link
Contributor

If I win what happens?

@ShaMan123
Copy link
Contributor

I don't like cocacola

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

I added the test in master, as soon as this is merged you can verify if the swap works, and in case do it.

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

I don't like cocacola

Any food in budget for the loser is fine.

@asturur
Copy link
Member Author

asturur commented Sep 3, 2022

I wrote sendPointRelativeToCanvas so a dev could interact with HTML objects in the dom from a nested object for example. We can trash it completely and I'll redo it later.

Ok i didn't know or i didn't remember this.
Offering an anchor point for html elements is doable as long there is a strategy before ( like where do we expect the dev to append the nodes? because if they are inside the canvas container there is need of any transform )

@asturur asturur merged commit 243d028 into master Sep 3, 2022
@ShaMan123
Copy link
Contributor

Offering an anchor point for html elements is doable as long there is a strategy before ( like where do we expect the dev to append the nodes? because if they are inside the canvas container there is need of any transform )

Good point
I'll reiterate

@asturur asturur deleted the see-where-i-broke branch September 11, 2022 23:02
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
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

2 participants