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

Relative URL / CSS fixes #37

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Relative URL / CSS fixes #37

wants to merge 4 commits into from

Conversation

peter-dolkens
Copy link

Adds support for url( syntax in CSS, and also support for relative URLs is extended.

Adds support for PageAsPDF - Legacy WebForms implementation of ViewAsPdf.
Adds support for HtmlAsPDF - Raw String implementation of ViewAsPdf.

Rotativa.Demo\TestWebForms.aspx demonstrates use of PageAsPDF (also included in Index page)

Courtesy Deepend Sydney.

@webgio
Copy link
Owner

webgio commented Aug 26, 2013

Sorry for the late reply, but didn't had much time lately. Some thoughts:

  • What does the adoption of regex solve in ViewAsPdf? Do you have a test to reproduce the scenario?
  • I updated to vs2012 too but didn't commit, did it today.
  • Not sure WebForms support belongs to Rotativa package. Would prefer to have a separate Rotativa.WebForms, perhaps depending on Rotativa.Core, that Rotativa would depend on too.
  • Don't really like HtmlAsPdf. It's not an implementation of AsPdfResultBase. Some refactoring of old code would be needed to encapsulate code not dependent on mvc (BuildPdf) on a separate class/assembly.

I will make some refactoring to separate wkhtmltopdf wrapper from MVC code. Then supporting WebForms will be cleaner.

@peter-dolkens
Copy link
Author

Hey,

I swapped to regex instead of adding a more replace lines, as I didn't
like repeatedly doing "string.replace" 6 times to achieve the current
functionality.
The new regex supports the url syntax in CSS too now, allowing for
background images, and other CSS file paths. I also updated the code to
support rooted and non-rooted paths (/image.png vs image.png)

HtmlAsPdf came along as a freeby when trying to add WebForms support for
a legacy site we had, not particularly attached to it.

There's also an MVC4 version which references all the MVC3 files, just
with MVC4 assembly references in the csproj file. Felt a little dirty doing
things like that, but it worked for our 2nd use-case, while maintaining
backwards compatability.

I can take a look at splitting WebForms support out to a separate
project. I was trying to minimize code duplication, and refactoring, and to
isolate the webforms code, I'd have had to pull a bunch of logic out of the
MVC3 project too. If you'd prefer it in a separate project, I'm sure I can
arrange that next time I'm working on the project that required this
component.

Thanks for the feedback. I haven't done too much open-source stuff, and
only just started using git, so if I'm doing anything wrong, let me know
how to improve my OS workflow.

Cheers,
Pete

On Tue, Aug 27, 2013 at 12:52 AM, Giorgio Bozio [email protected]:

Sorry for the late reply, but didn't had much time lately. Some thoughts:

What does the adoption of regex solve in ViewAsPdf? Do you have a test
to reproduce the scenario?

I updated to vs2012 too but didn't commit, did it today.

Not sure WebForms support belongs to Rotativa package. Would prefer to
have a separate Rotativa.WebForms, perhaps depending on Rotativa.Core, that
Rotativa would depend on too.

Don't really like HtmlAsPdf. It's not an implementation of
AsPdfResultBase. Some refactoring of old code would be needed to
encapsulate code not dependent on mvc (BuildPdf) on a separate
class/assembly.

I will make some refactoring to separate wkhtmltopdf wrapper from MVC
code. Then supporting WebForms will be cleaner.


Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-23267750
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants