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

Define logo-file as parameter #168

Merged
merged 6 commits into from
Mar 18, 2021
Merged

Conversation

BigBadBassMan
Copy link
Contributor

Hi,
since the logo is passed to the pdf generator separately (outside the template), it should be configurable by users, so it can be replaced with the actual businesses logo.

This PR introduces 2 parameters, one holding the bundle-path to the default logo, and a second one, which can be set by an .env var defaulting to the first path, if not set.
With this 2 step approach, this PR is fully backwards compatible.

In addition, the flex recipe would need an update along with the docs to reflect this changes.

add invoincing-logo-file parameter with defaults to bundle provided file
use parameter for logo file instead of hard coded path
@BigBadBassMan BigBadBassMan requested a review from a team as a code owner January 3, 2020 13:05
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hey @BigBadBassMan,

Thanks for your contribution and welcome to our community ;) Could you adjust your code to my suggestion?

src/Resources/config/services.xml Outdated Show resolved Hide resolved
@BigBadBassMan
Copy link
Contributor Author

I have no idea why the travis tests failed, local tests succeeded. Anyway, I'd love to see this merged, if there are no more blockers from your side.

@lchrusciel
Copy link
Member

Hey @BigBadBassMan, we cannot merge anything until the build is green. Someone has to take a look into the master build and fix it before we will be able to merge this PR.

@BigBadBassMan
Copy link
Contributor Author

Hey @BigBadBassMan, we cannot merge anything until the build is green. Someone has to take a look into the master build and fix it before we will be able to merge this PR.

I understand, perhaps you could trigger a re-test on travis?
It seems to me that this may be a transient failure, since I didn't change a single line in the tests, and my local tests (specifically the ecs and phpspec tests) run fine. The failure seems not to be related to my changes.

@lchrusciel
Copy link
Member

I've restarted the build. However, the master is failing one way or another: https://travis-ci.org/github/Sylius/InvoicingPlugin/builds/676556586

@BigBadBassMan
Copy link
Contributor Author

Hi @lchrusciel , I'm finally back on this, would you care to have another look (and possibly accept ;-) ) my PR?

@GSadee GSadee merged commit 1f662c1 into Sylius:master Mar 18, 2021
@GSadee
Copy link
Member

GSadee commented Mar 18, 2021

Thanks, @BigBadBassMan! 🥇

GSadee added a commit to Sylius/RefundPlugin that referenced this pull request Jun 14, 2021
This PR was merged into the 1.0-dev branch.

Discussion
----------

Inspired by PR made on Invoicing plugin(Sylius/InvoicingPlugin#168) I decided to add logo as parameter to Refund plugin, because almost every user needs to replace the logo.

Commits
-------

2f35e27 Logo as parameter
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.

3 participants