-
Notifications
You must be signed in to change notification settings - Fork 255
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
(GH-285) Add Control Margins #286
Conversation
LGTM |
@wjk are you able to sign the CLA? We are going to need this in order to pull in this suggestion. Also, any chance you can rebase your changes to include the issue number in the commit messages, as per the contributing.md document? |
@gep13: The CLA has been signed. In addition, I will rebase the changes once I have access to my Windows PC in a few hours. Thanks! |
@gep13 @RichiCoder1 Any update? I am especially curious why the AppVeyor CI build fails, yet the TeamCity CI build does not. If there are any more changes you want me to make before this is merged, please let me know. |
@wjk no, nothing else is required from you here, we just need to work to pull this in. I am tied up with a few other things, and I know @RichiCoder1 is working on some refactoring to the underlying usage of Chocolatey within ChocolateyGUI. Once we are both clear of those things, we will definitely be pulling this in. |
@wjk don't worry about the failing AppVeyor build. We do certain things on AppVeyor that we don't on the TeamCity build (mainly because we can) and there are some issues with the tooling that we are using. |
I'll look into getting this merged today! Def shouldn't let this sit :) |
@wjk thank you very much for this submission, and please accept my apologies for not getting back to you sooner, things have been a little hectic. Bottom line is, I have made some changes to the develop branch, in order to get the build working again on AppVeyor (it basically broke as a result of some changes that were made). Would it be possible for you to rebase this PR on the latest develop branch, and then I can look to getting this pulled in. Thanks again! |
@gep13 I’m sorry, but how exactly should I do this? I’ve never had to rebase a PR before. 😄 |
@wjk thanks for getting back to me. The steps would be as follow...
Hope that makes sense, if not, just let me know. Thanks again! |
@gep13 Done! Thank you! |
Awesome! Thanks for your contribution :) |
(GH-285) Add Control Margins
Fixes #285 |
I used a four-pixel margin to the controls, as I thought that looked good. In addition, I adjusted some of the thicknesses of the margins to not add extra padding where there already was some applied from the parent control. (This is why you see
Thickness="0,4,0,4"
in the below diff.) If you disagree on the amount of padding applied, please comment and I will revise my PR.