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

feat: allow any type of processor classes #50

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Conversation

Cruiser13
Copy link
Contributor

@Cruiser13 Cruiser13 commented Oct 25, 2023

Allows any processor classes as discussed in #44

Tagged services and a service locator would be the better way but overpowered for this use case I think. Let me know your thoughts.

Thank you for considering the PR.

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Cruiser13
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@kingjia90 kingjia90 linked an issue Oct 27, 2023 that may be closed by this pull request
@Cruiser13
Copy link
Contributor Author

@kingjia90 I see that a new release has been created without this PR. Are there any changes to be made?

@kingjia90
Copy link
Contributor

@Cruiser13 Unfortunately this is PR was not being reviewed yet and not in the main pipeline.

The changes looks fine so far, at least as quick fix, but maybe lacks a way to provide the settings by GUI eg.

select: function(combo, record) {
this.pdfReactorSettings.hide();
this.gotenbergSettings.hide();
this.chromiumSettings.hide();
if(combo.getValue() == "pdfreactor") {
this.pdfReactorSettings.show();
} else if(combo.getValue() == "chromium") {
this.chromiumSettings.show();
} else if(combo.getValue() == "gotenberg") {
this.gotenbergSettings.show();
}

@Cruiser13
Copy link
Contributor Author

@Cruiser13 Unfortunately this is PR was not being reviewed yet and not in the main pipeline.

The changes looks fine so far, at least as quick fix, but maybe lacks a way to provide the settings by GUI eg.

select: function(combo, record) {
this.pdfReactorSettings.hide();
this.gotenbergSettings.hide();
this.chromiumSettings.hide();
if(combo.getValue() == "pdfreactor") {
this.pdfReactorSettings.show();
} else if(combo.getValue() == "chromium") {
this.chromiumSettings.show();
} else if(combo.getValue() == "gotenberg") {
this.gotenbergSettings.show();
}

Thanks for letting me know. Since we can not know which settings the custom processor has I don't see a good way to add them here. And you can override the JS in your custom processor anyway. We re-implemented the wkhtmltopdf processor with these changes and simply supplied the web2print options in the class. Works fine.

@Cruiser13
Copy link
Contributor Author

@kingjia90 what do you think?

@kingjia90 kingjia90 added this to the 1.2.0 milestone Dec 21, 2023
@kingjia90 kingjia90 merged commit 8fd0e3e into pimcore:1.x Dec 21, 2023
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ease adding of new processors
2 participants