-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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): controls #8400
chore(TS): controls #8400
Conversation
progress progress(): position major progress fix(): flip cleanup no multiply good state! fix(): successive skewing cleaup
commited @luizzappa suggestion
return this.canvas.viewportTransform; | ||
} | ||
return iMatrix.concat() as TMat2D; | ||
return this.canvas?.viewportTransform || (iMatrix.concat() as TMat2D); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i m not sure we are transpiling those yet.
But the issue is that if undefined is not involved those becomes longer.
if typeof canvas !== 'undefined'
rather than a simple if this.canvas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please elaborate
I lack the knowledge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the diff between this syntax and obj?.[key]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none, but where we use obj?.[key] we were already looking for undefined ( ex: the overrideStyle object ). Here we want just to check if canvas is falsy in any way.
obj.first?.second
const temp = obj.first;
const nestedProp =
temp === null || temp === undefined ? undefined : temp.second;
If we land on supported browsers that do not support the ?.
all those equality check get added in the codebase. I just don't like to think all that stuff gets added in.
Not a big issue anyway
Seems all good to me apart:
|
Regarding the tasks you proposed , piping the function is a well known pattern as good as another, there is no reason to change it, it allows for reusability, keeps functions with a single concerns, is newish ( added in 4.0 ) i don't see the reason to change it, is just added chaos. Regarding sharing or no, that is really a no. I m totaly happy with a control set that is able to render itslef moving from object to object rather than many copies of it one per object. |
We will remain disagreed on this topic |
curAngle = Math.atan2(y - pivotPoint.y, x - pivotPoint.x); | ||
let angle = radiansToDegrees(curAngle - lastAngle + theta); | ||
|
||
if (target.snapAngle && target.snapAngle > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added isLocked
util, not very useful but at least it makes it simple to find all the locking logic
Extracting to a wrapper will cause too many unnecessary code changes
@asturur I have a compromise. class ControlSet {
defaultControls: Record<string, Control>;
controls: Record<string, Control>;
constructor(controls: Record<string, Control> = {}) {
this.defaultControls = controls;
}
getControl(key: string) {
return this.controls[key] || this.defaultControls[key];
}
setControl(key: string, control: Control) {
this.controls[key] = control;
}
} This is a win win solution. And I can fix the derivation of textbox controls with this approach |
A win-win-win you-me-textbox class ControlSet {
defaultControls: Record<string, Control> | ControlSet;
controls: Record<string, Control>;
constructor(controls: Record<string, Control> | ControlSet = {}) {
this.defaultControls = controls;
}
getControl(key: string): Control {
return (
this.controls[key] ||
(this.defaultControls instanceof ControlSet
? this.defaultControls.getControl(key)
: this.defaultControls[key])
);
}
setControl(key: string, control: Control) {
this.controls[key] = control;
}
}
const objectControls = new ControlSet(defaultControls);
const textboxControls = new ControlSet(objectControls);
textboxControls.setControl('mr', new Control({}));
objectControls.setControl('a', new Control({}));
textboxControls.getControl('a'); And then on each Object we do the same so when using new ControlSet(objectControls)
new ControlSet(textboxControls) |
It also allows use to move control logic from interactivity. class BoundControlSet {
constructor(controlSet: ControlSet, object: FabricObject) {
this.controls = controlSet;
this.object = object;
}
getVisibility(...) {
}
setVisibility(...) {
}
} This is a better pattern IMO for making interactivity optional and typed. We encapsulate the logic in dedicated classes instead on FabricObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of defaultControls
I think they should be assigned in the interactivity mixin.
And that textbox should subclass the mixin.
And that we use applyMixins for it instead of subclassing
Not sure though
Thoughts?
@ShaMan123 i want to do the TS migration, i don't want to do refactors. There is a lot of work to do and the more we do because we think is necessary, the later we can go back to features and website and docs I disagree on TS and classes the more i write them. peace. If we want to finish the ts migration we stick to that, we take notes on what we would like to see changed, we organize them and for now we move on head down on the goal Even the isLocked wrapper wasn't an idea i would have chased now. |
I understand. |
So if I understood you, my comments are throwing you off. |
How can i know that you put them in the PR because you want to discuss them somewhere else another day? |
let's skip getActionFromCorner check up and let's merge this. |
Motivation
Description
Migrating controls:
normalizePoint
intogetLocalPoint
Proposed Changes for Follow Up
fabricObject
will be a class property. This means controls will no longer be shared, another over discussed topic I advocate. In purpose I didn't do it in this PR (as @asturur requested not to over refactor) so we can migrate most of controls with no major disagreements. Something worth looking at is that there are 2 signaturesXXX(eventData, control, fabricObject)
andXXX(eventData, fabricObject, control)
. That is weird. But if we agree on bound controls both will becomeXXX(eventData)
. If not we should change them for consistency.