-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[stable9] fix searchbox background #623
Conversation
@jancborchardt, thanks for your PR! By analyzing the annotation information on this pull request, we identified @schiessle, @MorrisJobke and @PVince81 to be potential reviewers |
@MorrisJobke assigned you to check for the tests. :) |
I'm not your minion: You could at least check the output and fix the easy stuff https://travis-ci.org/nextcloud/server/jobs/147977236#L297 first. Then if you don't know how to fix the rest I could help you. |
@@ -314,6 +314,36 @@ public function testGetStylesheetWithOnlyColor() { | |||
->expects($this->at(1)) | |||
->method('getAppValue') | |||
->with('theming', 'color', '') | |||
<<<<<<< HEAD | |||
======= |
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.
looks-angry-to-Jan
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.
Well so what’s the deal – are the new additions the relevant ones, or should the change just be the empty one? You’re not my minion, but I’m also not a PHP coder. ;)
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.
That is, of course I looked at the conflicts and tried fixing them before opening this pull request. But I have no idea what parts of the PHP code are new and which old. Just poking in the dark by removing stuff here and other stuff there doesn’t seem like a good idea, that’s why I asked for your help.
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.
Should work now.
e24c7cb
to
176fbf9
Compare
@$this->assertEquals($expected, $this->themingController->getStylesheet()); | ||
} | ||
|
||
public function testGetStylesheetWithOnlyColorInvert() { |
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.
This is a duplicate unit test, as the color inverting was not backported to Nc9, but I guess this doesn't hurt.
I guess we can keep the unit tests, even if they are kind of duplicate. 👍 from my side |
Tested and works 👍 |
Thanks for your help @MorrisJobke! :) Maybe a talk on getting into writing tests at the Nextcloud Conference would be cool ;) |
@MorrisJobke can you fix the unit tests? I don’t know which exact features are in there to be tested at that point, like which have been backported. Also cc @juliushaertl for info since some of the inversion stuff etc was done by you. :)
Please review @nextcloud/designers @Mar1u5 @Bugsbane @williambargent @juliushaertl @MorrisJobke