Skip to content
This repository was archived by the owner on Dec 19, 2022. It is now read-only.

Converting to a full jsPDF-style plugin#8

Open
bgribaudo wants to merge 5 commits intocbix:masterfrom
bgribaudo:jsPDFStyle
Open

Converting to a full jsPDF-style plugin#8
bgribaudo wants to merge 5 commits intocbix:masterfrom
bgribaudo:jsPDFStyle

Conversation

@bgribaudo
Copy link
Contributor

  • Wrapped code inside anonymous function.
  • Renamed file to match jsPDF file name format.

@cbix
Copy link
Owner

cbix commented May 24, 2014

Hi,
Looks good! But is it backwards compatible?

@bgribaudo
Copy link
Contributor Author

Yes. :-( The SVG-to-PDF conversion method call is changed from
svgElementToPdf(svgString, pdf, options);
to
pdf.addSVG(svgString, x_offset, y_offset, options);.
This matches jsPDF's style.

Is this a problem?

@diegocr
Copy link

diegocr commented May 27, 2014

I think the problem is that svgElementToPdf is no longer global so you're breaking the backward compatibility with existing code doing that.

@bgribaudo
Copy link
Contributor Author

Yes, that's a correct assessment.

However, before we implement some kind of backwards compatibility, it might be best to get direction from @cbix on whether asking users to slightly restyle their calls to svgToPdf so that they match jsPDF's style is okay or a bad thing?

If backwards compatibility is necessary, then maybe it can be turned on via some kind of configuration variable. This way, out of the box, svgToPdf will match jsPDF's plugin standards.

@diegocr
Copy link

diegocr commented May 28, 2014

maybe it can be turned on via some kind of configuration variable.

This will break compatibility as well if existing code needs to be modified to turn that on.

svgToPdf will match jsPDF's plugin standards.

It does already. The PR i've submitted on your fork should take care of initialization for:

  • NodeJS
  • RequireJS
  • jsPDF
  • Backwards compatibility

I think that might be overkill, but at least NodeJS was mentioned on the Roadmap here. Ideally we should add NodeJS support on jsPDF itself for new code.

@bgribaudo
Copy link
Contributor Author

Thank you for the pull request, @diegocr. My goal is for my jsPDFStyle branch to be merged back into @cbix's repository, so I think it would be best to wait for @cbix to share his decision on backwards compatibility before proceeding further on this.

Florian, to summarize the outstanding question: Is it okay to remove the method svgElementToPdf?

  • The cost of a yes answer is loss of backwards compatibility.
  • The cost of a no answer is the code complexity required to make svgElementToPdf and the jsPDF plugin way of doing things both work.

My vote, considering the fact that this is an extension for jsPDF and so (hopefully) most users should be used to doing things the jsPDF way, is to drop backward compatibility. The cost of asking users who want to upgrade their svgToPDF extension to switch to doing things the jsPDF way seems small.

@cbix
Copy link
Owner

cbix commented Jun 6, 2014

Hey guys, sorry for my ignorance, actually the backwards compatibility has less importance over stability and good practice like no global functions etc., so I think this is not only ok but a big enhancement! Thanks, I will merge as soon as tested :)

@bgribaudo
Copy link
Contributor Author

Sounds great, @cbix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants