Skip to content

[WIP] Add export density to canvas renderer#146

Open
lucasdinonolte wants to merge 9 commits into
pixel-density-exportsfrom
crisp-canvas-previews
Open

[WIP] Add export density to canvas renderer#146
lucasdinonolte wants to merge 9 commits into
pixel-density-exportsfrom
crisp-canvas-previews

Conversation

@lucasdinonolte

Copy link
Copy Markdown
Contributor
  • Makes canvas previews crisp on retina displays
  • Allows users to choose the desired pixel density before exporting

@runemadsen

Copy link
Copy Markdown
Contributor

I was just thinking about this, and I think this is the right implementation. We should make sure that the getCanvas function throws an error if it doesn't have the needed width and height, whether that comes from inputs, the getCanvas arguments, or both. A nice error message with a call for the user to add the dimensions would be very helpful.

@lucasdinonolte

Copy link
Copy Markdown
Contributor Author

I was just thinking about this, and I think this is the right implementation. We should make sure that the getCanvas function throws an error if it doesn't have the needed width and height, whether that comes from inputs, the getCanvas arguments, or both. A nice error message with a call for the user to add the dimensions would be very helpful.

I had the same notion and added some first drafts for error messages. https://github.com/designsystemsinternational/mechanic/pull/146/files#diff-cea56d01b8f19fd6436a19f2b4bbd9d2258c6edbeef3df917df2026fcf1ed520R82 😊

Comment thread packages/engine-canvas/index.js Outdated
Comment thread packages/engine-canvas/index.js Outdated
Comment thread packages/engine-canvas/index.js Outdated
Comment thread packages/dsi-logo-maker/functions/textAnimatedCanvas/index.js
state: mechanic.functionState,
setState: onSetState,
},
getCanvas: ({ width = null, height = null } = {}) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's a bit odd from the interface that we ask for the canvas and context from this function and then are supposed to give it back through frame and done. Would it make sense to keep a reference inside the engine and then use it when frame and done get called?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like that idea!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I like it. I tried it and it's very nice. The only situation this is problematic is, when you also want to pass a name to onDone. So I changed done to accept an object as arguments. Let me know what you think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do like it! It does made me realize there's no mention in the documentation about the name argument for done. Maybe this is an opportunity to streamline arguments for the frame and done functions and leave all of them as single argument functions on all engines. Can be done in a separate PR tho.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm all in for this!

lucasdinonolte and others added 3 commits September 16, 2022 11:24
Co-authored-by: Fernando Alberto Florenzano Hernández <f.florenzano94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants