chore(TS): canvas options #9140
Conversation
|
Build Stats
|
| assert.ok('backgroundImage' in canvas); | ||
| assert.ok('overlayImage' in canvas); |
There was a problem hiding this comment.
do you know why those are not true anymore?
There was a problem hiding this comment.
maybe canvas.hasOwnProperty would work? how do wecheck we don't accidentally remove stuff from the classes? this was meant to do this only
There was a problem hiding this comment.
No need
I wrote in the description that I changed the type from null to undefined.
So there was no sense in setting these fields to undefined in default props so I removed them.
That is all.
There was a problem hiding this comment.
yeah if are not set the class doesn't know about them. that is bad and rise some questions.
If the class are better because differently from objects they are all predefined and so the compiler has no surprises on members and methods, i wonder what the optional properties do. Those are just added at runtime?
There was a problem hiding this comment.
Not sure.
But that is the point of declare, isn't it?
Without declare it is defined as void 0.
But then we have problems in initialization.
That can be fixed of coursr but will require a lot if refactoring
| * @default | ||
| * @since fabric 4.0 // changed name and default value | ||
| */ | ||
| uniformScaling: boolean; |
There was a problem hiding this comment.
In retrospective i don't like the idea of moving the documentation out of the classes themselves. I understand it makes files shorter but it seems the wrong solution.
\I know we did this already and is not new, but now i really dislike it.
Opening a class and being able to read is important to me
There was a problem hiding this comment.
If you hover over a prop it shows the jsdoc.
I like it but that is why I kept it in separate commits... because I wanted to give you an easy way out.
There was a problem hiding this comment.
no if we go out we change them all. For now we keep consistency.
I know hovering works but reading is reading. It is also a vscode thing not an universal one
There was a problem hiding this comment.
So should I revert or not?
There was a problem hiding this comment.
Should the jsdoc exist on the options as well?
For a dev the important place is on options so when creating a canvas ir object they see what it can do.
There was a problem hiding this comment.
I agree with @asturur, better a big module with everything related than death-by-thousand-files. While vscode hover documentation helps, there are many other occasions where it's useful to have things colocated in the same file, e.g. PR reviewing or debugging.
There was a problem hiding this comment.
I'd recommend this book where the topic of "deep" modules is explained: https://www.amazon.com/Philosophy-Software-Design-John-Ousterhout/dp/1732102201, i.e. a deep module with the relevant implementations all together (as far as the exposed public API is small) is better than many shallow modules that you need to put together in your mind in order to understand.
|
Why was this flag removed @ShaMan123 ? We use it for instance so that when generating a snapshot of the canvas, some elements like the grid are not rendered because the canvas is not interactive |
|
I removed the |
What do you mean? In TS (and JS) the |
Motivation
Description
First merge #9139
Typing canvas options
Changes
interactiveflag 0d0ce13backgroundImage,overlayas undefined instead of null and removed from default values with clipPathGist
2 difficult commits 86927bd 6791d74
In Action