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

Theming: Generate different favicon sizes #6460

Merged
merged 3 commits into from
Nov 13, 2017

Conversation

juliushaertl
Copy link
Member

This should avoid resizing done by browsers and will fix #5193

Signed-off-by: Julius Haertl jus@bitgrid.net

@nextcloud/theming

@juliushaertl juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 13, 2017
@codecov
Copy link

codecov bot commented Sep 17, 2017

Codecov Report

Merging #6460 into master will increase coverage by <.01%.
The diff coverage is 95%.

@@             Coverage Diff              @@
##             master    #6460      +/-   ##
============================================
+ Coverage     50.72%   50.73%   +<.01%     
- Complexity    24478    24479       +1     
============================================
  Files          1581     1581              
  Lines         93578    93596      +18     
  Branches       1359     1359              
============================================
+ Hits          47471    47484      +13     
- Misses        46107    46112       +5
Impacted Files Coverage Δ Complexity Δ
apps/theming/lib/IconBuilder.php 67.96% <95%> (+4.33%) 25 <0> (+1) ⬆️
core/js/js.js 61.27% <0%> (-0.56%) 0% <0%> (ø)
lib/private/Server.php 83.29% <0%> (-0.13%) 125% <0%> (ø)
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

@juliushaertl juliushaertl force-pushed the theming-favicon-generated-sizes branch 3 times, most recently from 4c5b87b to 70d4137 Compare September 23, 2017 12:22
@juliushaertl
Copy link
Member Author

Tests are also fixed now. Please review @nextcloud/theming @nickvergessen

@juliushaertl juliushaertl added this to the Nextcloud 13 milestone Sep 23, 2017
@juliushaertl juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 3, 2017
@juliushaertl
Copy link
Member Author

bump ... please review @nextcloud/theming

@skjnldsv
Copy link
Member

skjnldsv commented Nov 7, 2017

How can we test this? :)

@juliushaertl
Copy link
Member Author

@skjnldsv You need imagemagick enabled in your webserver setup. Then the endpoint http://localhost/index.php/apps/theming/favicon/core should return an ico file with different sizes.

@MorrisJobke
Copy link
Member

@skjnldsv You need imagemagick enabled in your webserver setup. Then the endpoint http://localhost/index.php/apps/theming/favicon/core should return an ico file with different sizes.

#7100

@skjnldsv
Copy link
Member

skjnldsv commented Nov 7, 2017

@skjnldsv You need imagemagick enabled in your webserver setup. Then the endpoint http://localhost/index.php/apps/theming/favicon/core should return an ico file with different sizes.

I did, but I only get one picture. 32x32

@juliushaertl
Copy link
Member Author

@skjnldsv I think most browsers/image viewers will show only a single image. Opening in gimp should show up the different sizes as single layers.

@skjnldsv
Copy link
Member

skjnldsv commented Nov 9, 2017

Hum, I only have one layer. Header is set and working.
capture d ecran_2017-11-09_14-33-41

@juliushaertl
Copy link
Member Author

Can you change the color and try it again? I guess we need to trigger the regeneration when updating.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Perfect!

@MorrisJobke
Copy link
Member

I like it and it works here, but somehow postgres fails hard:

Error while trying to create admin user: An exception occurred while executing 'SELECT min_value, increment_by FROM "oc_personal_settings_id_seq"':
21s
14

21s
15
SQLSTATE[42703]: Undefined column: 7 ERROR:  column "min_value" does not exist
21s
16
LINE 1: SELECT min_value, increment_by FROM "oc_personal_settings_id...
21s
17
               ^
21s
18
 -> 

…browser

fixes #5193

Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

@MorrisJobke Tests run fine locally. I rebased and pushed again. Let's see if that one still fails.

@nickvergessen
Copy link
Member

nickvergessen commented Nov 13, 2017

I like it and it works here, but somehow postgres fails hard:

@MorrisJobke Postgres 10 is still not supported: #5930

@MorrisJobke MorrisJobke merged commit 26bcf40 into master Nov 13, 2017
@MorrisJobke MorrisJobke deleted the theming-favicon-generated-sizes branch November 13, 2017 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Favicon color is a bit off?
4 participants