Skip to content

Conversation

@ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Aug 30, 2023

Motivation

I was in a need for pdf output

Description

Utilizing node canvas' native pdf/svg output by accepting a ctx as arg for toCanvasElement

first merge #9112 for registering fonts

I have come up with a different approach that can be achieved by 2 subclasses: https://codesandbox.io/p/devbox/fabric-node-sandbox-forked-mg9222?workspaceId=d29ca40f-0679-47bb-8fc8-429da561f9e5

Changes

  • added additonal ctx optional arg/option
  • types
  • tests

Gist

In Action

https://codesandbox.io/p/devbox/modern-waterfall-glhq62

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

Build Stats

file / KB (diff) bundled minified
fabric 909.896 (+0.828) 304.767 (+0.359)

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

ready almost

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

need to add fonts in node

@ShaMan123 ShaMan123 requested a review from asturur August 30, 2023 09:08
@ShaMan123
Copy link
Contributor Author

@melchiar I think you would be interested in this

multiplier = 1,
enableRetinaScaling = false,
...options
}: TDataUrlOptions = {}): string {
Copy link
Member

Choose a reason for hiding this comment

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

to me it doens't make sense to specify a ctx here.
Can we releage this to a specific node item?
If we need to export something in PDF/SVG using node-canvas, shouldn't we add a method that is specific to that and return something related to that?

We don't have code size issues in node and we are not bound to bundling issues.
Having toCanvasElement that return a canvas element that is good for pdf/svg export buy only if we previously created a specific ctx sounds complicated.

Let's have a toNodePDF and a toNodeSVG ( or whatever it can be called to avoid confusion with current toSVG ) in the node classes.

Take also care that moving away from node-canvas is still a remote option and if there are other libraries that do not require building or that are more canvas compliant, is not granted they will have a pdf or svg export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is why I didn't expose toPDF etc. So we are not bound to node canvas.

Suggest a different way?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Nov 19, 2023

I have come up with a different approach that can be achieved by 2 subclasses: https://codesandbox.io/p/sandbox/fabric-node-sandbox-forked-8hwny3?file=%2Fsrc%2Findex.mjs%3A9%2C1-22%2C2

This is a really good demo for the website... but that must use codesandbox to run a container... another reason to use codesandbox @asturur

@ShaMan123 ShaMan123 closed this Feb 9, 2024
@ShaMan123 ShaMan123 reopened this Feb 9, 2024
init
Update typedefs.ts

better test

pass ctx

update CHANGELOG.md
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

rebased to head

@ShaMan123 ShaMan123 added the stale Issue marked as stale by the stale bot label May 7, 2024
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented May 8, 2024

Anyone interested in this great solution bear in mind that it is also possible to achieve it on the client by using canvas API svg renderers.

@ShaMan123 ShaMan123 closed this May 8, 2024
@asturur asturur reopened this May 8, 2024
@stale stale bot removed the stale Issue marked as stale by the stale bot label Aug 28, 2024
@hamzawain7
Copy link

I love & need this. Can we please add this feature? Can I manually merge these changes in my fork till then? Are there any known issues?

@asturur
Copy link
Member

asturur commented Aug 29, 2024

This should be node only and not on the generic methods. Those will work only under node and there should be.

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.

4 participants