-
Notifications
You must be signed in to change notification settings - Fork 122
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
Contributors needed? #93
Comments
Hi Arjan,
Thanks for your contribution. I do need help and I plan on looking at those
PRs before the end of August. I'll be in touch with you so we can
understand how to collaborate. I don't have experience on open source
collaboration, I need to understand how to make it work.
…On Wed, Aug 14, 2024 at 9:38 AM Arjan ***@***.***> wrote:
At the moment there are 4 helpful open PRs (#54
<#54>, #55
<#55>, #90
<#90> and #92
<#92>) from people who
would like to advance this project, adding .NET 8 support, unit tests,
wkhtmltoimage support, new properties, documentation and configuration.
They are dating back to 2020. Do you need help in maintaining this project?
I'm using this project, so I would be happy to contribute.
—
Reply to this email directly, view it on GitHub
<#93>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCTPPNVCVXWXYORL3XA2DZRMCOZAVCNFSM6AAAAABMPYCKSGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ3DKMJSGAYDMMQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Thank you for your reaction @webgio and take your time! I could help by keeping the project up-to-date with new .NET versions, adding more documentation and responding to issues/PRs if able. If you like to have the final say in changes I could let you do the merges. The #92 PR contains the changes proposed in the other PRs as well, as I needed both .NET 8 and image support, while refactoring the code a bit to prevent code duplication. I added the Zoom property as well because there was a conflict in that PR. The PR also solves #88. Issue #17 and #85 can also be closed after the merge because we allow people to disable setting the base URL. So we could close 4 PRs and 3 issues by reviewing and merging 1 PR. In the future I need to make the PRs smaller. |
Hi @webgio, how would you like to go forward? Will you have time for some necessary maintenance in the near future so this project could stay relevant? If yes, that would be perfect, if no, that is understandable. If you don't have the time anymore, could you make a decision on how this project could stay up-to-date without your input? It could also be a temporary solution up until when you have time again. I don't want to fork this project, forcing people to switch packages and ending up with two development paths if you continue later. I'm happy with any direction you choose that would keep the project going. In the future I would like to add additional drivers to this project, as wkhtmltopdf is archived and no longer maintained. Then this project will make it easy to add PDF capabilities to any .NET project, with a PDF generator of your choice. |
Hi @ArjanKw, I had issues reviewing your PR, could you check my comments? I don't have much time but I think we can merge the PR pretty quickly if I'm able to run it locally. |
@ArjanKw I thought the comments were visible, sorry. I started the review in August... so I'm trying to get back to it and remember what was the point of the issue. I was trying to run tests but I couldn't understand how you run both .net 6 and .net 8 versions on different ports. I think everything else looks good. |
No problem. In line with the rest of the code I commented out the Asp.net 6 variant and added the Asp.net 8 variant. The Github review makes it seem in the review page as if both Asp.net 6 and Asp.net 8 are not commented, but this is the end result: //[InlineData("http://localhost:64310", "Asp.net core 2.0")]
//[InlineData("https://localhost:44375", "Asp.net core 3.1")]
//[InlineData("https://localhost:7059", "Asp.net 6")]
[InlineData("https://localhost:56246", "Asp.net 8")] 56246 is the port where the DemoApp is running on locally on my machine. That is different for everyone because the launchSettings.json is an ignored file in Git. If you want the port to be the same for everyone, we could unignore the launchSettings.json file. |
At the moment there are 4 helpful open PRs (#54, #55, #90 and #92) from people who would like to advance this project, adding .NET 8 support, unit tests, wkhtmltoimage support, new properties, documentation and configuration. They are dating back to 2020. Do you need help in maintaining this project? I'm using this project, so I would be happy to contribute.
The text was updated successfully, but these errors were encountered: