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.

Sign up for GitHub

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

Jump to bottom

Prefer custom theme over the theming app #5070

Merged
merged 1 commit into from
Jul 13, 2017
Merged

Prefer custom theme over the theming app #5070

merged 1 commit into from
Jul 13, 2017

Conversation

juliusknorr
Copy link
Member

This PR will prevent the theming app from loading its CSS/JavaScript/Images in case a custom theme is used.

This occured in #5036 and #3094

I think this is better default behavior.

@juliusknorr juliusknorr added this to the Nextcloud 13 milestone May 23, 2017
nickvergessen
nickvergessen reviewed May 30, 2017
View reviewed changes
lib/private/URLGenerator.php Outdated
$path = \OC::$WEBROOT . "/themes/$theme/apps/$app/img/$image";
} elseif (!file_exists(\OC::$SERVERROOT . "/themes/$theme/apps/$app/img/$basename.svg")
&& file_exists(\OC::$SERVERROOT . "/themes/$theme/apps/$app/img/$basename.png")) {
$path = \OC::$WEBROOT . "/themes/$theme/apps/$app/img/$basename.png";
Copy link
Member

Choose a reason for hiding this comment

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

Accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fixed that. Thanks.

@codecov
Copy link

codecov bot commented May 30, 2017 •
edited
Loading

Codecov Report

Merging #5070 into master will decrease coverage by <.01%.
The diff coverage is 27.27%.

@@             Coverage Diff              @@
##             master    #5070      +/-   ##
============================================
- Coverage     53.77%   53.76%   -0.01%     
+ Complexity    22744    22742       -2     
============================================
  Files          1405     1405              
  Lines         86875    86881       +6     
  Branches       1328     1328              
============================================
- Hits          46715    46711       -4     
- Misses        40160    40170      +10
Impacted Files Coverage Δ Complexity Δ
apps/theming/appinfo/app.php 0% <0%> (ø) 0 <0> (ø) ⬇️
core/js/mimetype.js 91.11% <100%> (ø) 0 <0> (ø) ⬇️
apps/theming/lib/Util.php 92.3% <100%> (+0.52%) 28 <2> (+2) ⬆️
lib/private/URLGenerator.php 76% <37.5%> (-1.64%) 60 <0> (-4)
apps/comments/lib/EventHandler.php 79.16% <0%> (-8.34%) 7% <0%> (ø)
core/js/js.js 61.27% <0%> (-0.56%) 0% <0%> (ø)
lib/private/Server.php 93.36% <0%> (-0.15%) 120% <0%> (ø)
lib/private/Security/CertificateManager.php 91.83% <0%> (+1.02%) 39% <0%> (ø) ⬇️

@MorrisJobke
Copy link
Member

@juliushaertl Conflicts.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 13, 2017
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 14, 2017
MorrisJobke
MorrisJobke approved these changes Jun 19, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Works

@MorrisJobke
Copy link
Member

Please review ;)

@nickvergessen
Copy link
Member

Works

@nickvergessen
Copy link
Member 

There were 2 failures:
1) OCA\Theming\Tests\UtilTest::testIsAlreadyThemedFalse
Expectation failed for method name is equal to <string:getSystemValue> when invoked 1 time(s)
Parameter 0 for invocation OCP\IConfig::getSystemValue('theme', '') does not match expected value.
'theme' does not match expected type "array".
/drone/src/github.com/nextcloud/server/apps/theming/lib/Util.php:208
/drone/src/github.com/nextcloud/server/apps/theming/tests/UtilTest.php:189
2) OCA\Theming\Tests\UtilTest::testIsAlreadyThemedTrue
Expectation failed for method name is equal to <string:getSystemValue> when invoked 1 time(s)
Parameter 0 for invocation OCP\IConfig::getSystemValue('theme', '') does not match expected value.
'theme' does not match expected type "array".
/drone/src/github.com/nextcloud/server/apps/theming/lib/Util.php:208
/drone/src/github.com/nextcloud/server/apps/theming/tests/UtilTest.php:199

nickvergessen
nickvergessen requested changes Jul 6, 2017
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Tests fail

@juliusknorr
Prefer custom theme over theming app
ce5ad7e 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@nickvergessen Fixed 😉

@nickvergessen nickvergessen merged commit e335121 into master Jul 13, 2017
@nickvergessen nickvergessen deleted the theming-vs-themes branch July 13, 2017 11:41
@MorrisJobke MorrisJobke mentioned this pull request Jul 13, 2017
Closed
@juliusknorr juliusknorr mentioned this pull request Sep 28, 2017
Closed
@MorrisJobke MorrisJobke mentioned this pull request Nov 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@LukasReschke LukasReschke Awaiting requested review from LukasReschke

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@schiessle schiessle Awaiting requested review from schiessle

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: theming
Projects
None yet
Milestone
Nextcloud 13
Development

Successfully merging this pull request may close these issues.
3 participants
@juliusknorr @MorrisJobke @nickvergessen