-
Notifications
You must be signed in to change notification settings - Fork 129
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
Install missing skin via composer on system initialization #254
Conversation
Ping |
@thomascube Do you see any technical reason not to merge this? (I'm ready to pick up the work, just want to know your opinion.) |
@pabzm No objections. Maybe even configure a non-default skin in the docker-compose.* tests to verify the process works? |
@bjalbor Could you try to extend |
@pabzm |
Hi there,
sorry for missing the message.
I'll try to find some time next week for taking care of adding you
suggestion.
Regards
Pablo Zmdl ***@***.***> schrieb am Fr., 8. Nov. 2024, 08:04:
… @bjalbor <https://github.com/bjalbor> Could you try to extend
tests/docker-compose.test-* so that they install a non-default skin, so
this feature is tested?
—
Reply to this email directly, view it on GitHub
<#254 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG7HJE5NZE4OODDCTXXAIHDZ7RPADAVCNFSM6AAAAABI6P3GOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRTHEYTQMZVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@bjalbor No pressure, we've got time! Thank you for the notice, and thank you for working on this! |
I hope everything is fine now. Sorry, if something is still missing. To be honest this is my very first contribution to a project in github 😊 |
@bjalbor Don't worry, it's an honour to be your first project! I'll have a look into the failing tests and will try to help you within the next days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the problem, which comes from code you copied, so it's not your fault at all.
Hooray for testing!
Thank you again for your contribution, and congratulations to your courage to post your first pull request!
I'm not sure, if I did it right, but I managed to bring my pull request to match the actual state. What I did: Synced and pulled my fork project, rebased/merged my local repo, tidied up and pushed back. Was that the right way or did I miss anything? Sorry for the inconvenience... |
@bjalbor You did it well, thank you! Since you merged the "master"-branch (instead of rebasing it), github now thinks that this PR wants to merge 31 commits – most of those are actually part of the "master"-branch already, though. I'll use a "squash-commit", to reduce the amount of commits to one. |
@bjalbor I allowed myself to resolve a conflict between your branch and the "master" branch, which came from merging another pull request. Curiously now Github nows that most of the commits don't need to be merged anymore... 🤷 |
@bjalbor Your first contribution is merged, congratulations! Well done! 🎉 If you have more ideas or see bugs, let us know! |
@pabzm Yippee, it finally worked. Thank you for your help in learning about this stuff. I certainly will come back if I find s.th. more to improve. |
Only classic skin ist included in core. So if you define ROUNDCUBEMAIL_SKIN to anything other, you will get an 404 (also see #243) unless you install missing skin. This contribution will install the missing skin on system startup.