Fix(StaticCanvas): Restrict and correct setDimensions typings. #9618
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
| | { | ||
| backstoreOnly?: true; | ||
| cssOnly?: false; | ||
| } | ||
| | { | ||
| backstoreOnly?: false; | ||
| cssOnly?: true; |
There was a problem hiding this comment.
| | { | |
| backstoreOnly?: true; | |
| cssOnly?: false; | |
| } | |
| | { | |
| backstoreOnly?: false; | |
| cssOnly?: true; | |
| | { | |
| backstoreOnly?: true; | |
| cssOnly?: never; | |
| } | |
| | { | |
| backstoreOnly?: never; | |
| cssOnly?: true; |
should be never or undefined, not sure
There was a problem hiding this comment.
i tried this, it gets angry. false is still a valid option you could pass, the important is that you don't do the double true.
src/canvas/StaticCanvas.ts
Outdated
| dimensions: Partial<TSize | CSSDimensions>, | ||
| options: TCanvasSizeOptions = {} |
There was a problem hiding this comment.
question about this
What happens if I pass a css dimension string with option cssOnly = false?
I think we should change the second arg to be a string option or merge it into the first arg.
setOnly?: 'css' | 'backstore'
There was a problem hiding this comment.
Or better off
css?: boolean = true,
backstore?: boolean = true,
OR add
cssWidth?: whatever,
cssHeight?: whatever,
There was a problem hiding this comment.
I prefer the last, explicit and clear
width: number,
height: number,
cssWidth?: whatever,
cssHeight?: whatever,
There was a problem hiding this comment.
yes with 4 width/height would be nice, but would be another breaking change.
Let's have this as a fix of the current situation and i ll open a different PR with the breaking change
There was a problem hiding this comment.
maybe i can just add cssWidth and cssHeight and deprecate the rest.
There was a problem hiding this comment.
What happens if I pass a css dimension string with option cssOnly = false?
This has been solved now with stricter typings
i was doing this https://tapto-designer.netlify.app/ and with the window resize event and setDimensions it was too laggy and also too many react issues with the resize observer, so i decided to go with the CSS and i noticed i wasn't possible anymore because of TS ( but worked if forced ). If you have a bunch of jpegs on your pc ( like 10 or more ) you can load them with add files ( is all local, data goes nowhere ) and you ll get a canvas per jpeg. Then you can resize the window and you will see is pretty smooth |
|
cssWidth and cssHeight are probably the way to go, and a specific PR should refactor the code around to make that possible |
| }); | ||
| if (!cssOnly) { | ||
| this._setDimensionsImpl(dimensions, options); | ||
| if (options && !options.cssOnly) { |
There was a problem hiding this comment.
@asturur this is a regressions
if no options are passed the canvas does not render
There was a problem hiding this comment.
Quoting: A typings PR should be only a typings PR
There was a problem hiding this comment.
@ShaMan123 you are using snarky comments since yesterday.
I don't like that.
This PR has been extensively looked by me and you and a bug slipped in.
It was a typing pr indeed, no logic was meant to change
There was a problem hiding this comment.
And the PR is titled as fix() and includes the word typings is not that i wrote chore(TS) and changed the code
Description
setDimensions can be used as:
When we want a responsive static canvas.
Current types have been restricted incorrectly to number only here:
4107721#diff-75d92bd48875f2c27e21818dc1794f3cfae0b91452086e34ce64fc09dc707e71L441