-
-
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
Fix(StaticCanvas): Restrict and correct setDimensions typings. #9618
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Build Stats
|
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 never knew this was an option
Curious to see how it works with window resizing
| { | ||
backstoreOnly?: true; | ||
cssOnly?: false; | ||
} | ||
| { | ||
backstoreOnly?: false; | ||
cssOnly?: true; |
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.
| { | |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better off
css?: boolean = true,
backstore?: boolean = true,
OR add
cssWidth?: whatever,
cssHeight?: whatever,
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 prefer the last, explicit and clear
width: number,
height: number,
cssWidth?: whatever,
cssHeight?: whatever,
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i can just add cssWidth and cssHeight and deprecate the rest.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asturur this is a regressions
if no options are passed the canvas does not render
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.
Quoting: A typings PR should be only a typings PR
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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