Skip to content

Conversation

@QJesus
Copy link

@QJesus QJesus commented Jul 20, 2022

  1. fabric.Text.text cannot be null\undefined
  2. undo_redo extension

@ShaMan123
Copy link
Contributor

fabric.Text.text cannot be null\undefined

Can be a standalone PR

@ShaMan123 ShaMan123 closed this Jul 22, 2022
@ShaMan123 ShaMan123 reopened this Jul 22, 2022
lib/undo_redo.js Outdated
Comment on lines 46 to 47
const ___setupCanvas = fabric.canvas.prototype.__setupCanvas;
fabric.canvas.prototype.__setupCanvas = function (serialized, enlivenedObjects, renderOnAddRemove, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ___setupCanvas = fabric.canvas.prototype.__setupCanvas;
fabric.canvas.prototype.__setupCanvas = function (serialized, enlivenedObjects, renderOnAddRemove, callback) {
const ___setupCanvas = fabric.Canvas.prototype.__setupCanvas;
fabric.Canvas.prototype.__setupCanvas = function (serialized, enlivenedObjects, renderOnAddRemove, callback) {

Nice lib idea! fabric.canvas should have uppercase C though

Copy link
Contributor

@melchiar melchiar left a comment

Choose a reason for hiding this comment

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

Can we improve some of these variable names. Names like t, v, and h aren't very human readable.

Some comments would be helpful as well to explain the purpose of each function and variables like undoredo.escape

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 23, 2022

I don't think history should be part of core.
IMO history is something that is heavily customized between different apps.
If it is hard to build such functionality I would suggest PRing to make fabric easier to develop such functionalities. That makes sense to me.
Regardless, saveObjectTransform isn't good enough. What about skewing? cacheProperties? stateProperties?
This re-introduces state management which we have decided to deprecate in #7920
And of course we are in the midst of TS migration, so the PR lockdown should be enforced. After that it might be very simple to develop this kind of functionality.

@ShaMan123
Copy link
Contributor

@QJesus I am very for contributions, hope I didn't put you down.
We are migrating to TS if you want to join the effort. And that should eventually open up a better way to write extensions for fabric.

@asturur
Copy link
Member

asturur commented Jul 25, 2022

Actually we wanted to think about a more simple way to tackle this, both on core and an external lib. We have to move forward with our schedule and as soon as the rollup pr is merged, we can get back to this.

@melchiar melchiar dismissed their stale review July 26, 2022 02:14

change of direction

@melchiar
Copy link
Contributor

@QJesus Thanks for the pr, however as asturur explained this isn't quite the direction we want to take with an undoredo library. I'm going to close this PR for now, but we can revisit adding an official undoredo component once v6 is out of the gate.

@melchiar melchiar closed this Jul 26, 2022
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.

4 participants