Skip to content
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

[WIP] Add export density to canvas renderer #146

Open
wants to merge 9 commits into
base: pixel-density-exports
Choose a base branch
from

Conversation

lucasdinonolte
Copy link
Contributor

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

@runemadsen
Copy link
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
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 😊

packages/engine-canvas/index.js Outdated Show resolved Hide resolved
packages/engine-canvas/index.js Outdated Show resolved Hide resolved
packages/engine-canvas/index.js Outdated Show resolved Hide resolved
state: mechanic.functionState,
setState: onSetState,
},
getCanvas: ({ width = null, height = null } = {}) => {
Copy link
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
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
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
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
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.

None yet

3 participants