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

Add favicons #1960

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Add favicons #1960

merged 1 commit into from
Dec 7, 2020

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Dec 4, 2020

Fixes #1947

All SVGs were created and converted to PNGs with Inkscape. The ICO version was converted using ImageMagick. For dimensions and styling I adhered to the favicons of nextcloud/mail.

All PNGs and SVGs were optimized using nextcloud/server/build/image-optimization.sh.

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny added enhancement New feature or request 3. to review Waiting for reviews labels Dec 4, 2020
@st3iny st3iny requested a review from skjnldsv December 4, 2020 17:07
@st3iny st3iny self-assigned this Dec 4, 2020
@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #1960 (0ef653f) into master (ecb4a05) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1960   +/-   ##
=========================================
  Coverage     67.53%   67.53%           
  Complexity      249      249           
=========================================
  Files            22       22           
  Lines           690      690           
=========================================
  Hits            466      466           
  Misses          224      224           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecb4a05...e2369cd. Read the comment docs.

@skjnldsv
Copy link
Member

skjnldsv commented Dec 5, 2020

They're supposed to be generated by the server for quite some times now. Thus why they're not here anymore.
I'm confused to what is happening 🤔
cc @juliushaertl, do you know a bit more?

@st3iny
Copy link
Member Author

st3iny commented Dec 5, 2020

I checked every app on my private instance and I see favicons only for the apps which explicitly ship their own. Could the auto generation be related to the theming app? I had to disable this app because it is not working well when using a s3 bucket as primary storage.

@skjnldsv
Copy link
Member

skjnldsv commented Dec 7, 2020

It requires imagick with svg support being enabled on the instance, maybe that is not the case here then:

I have imagick enabled and definitely don't have the contacts favicon 🤔
I think it's broken?

@skjnldsv skjnldsv merged commit b7053f5 into master Dec 7, 2020
@welcome
Copy link

welcome bot commented Dec 7, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/contacts/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-contacts and #nextcloud-dev on Freenode for a chat!

@skjnldsv skjnldsv deleted the enhancement/1947/add-favicons branch December 7, 2020 08:25
@juliusknorr
Copy link
Member

Hm, works for me:
image

@st3iny
Copy link
Member Author

st3iny commented Dec 19, 2020

@juliushaertl @skjnldsv FYI, the theming app is responsible for automatic favicon generation.

I enabled it on my Nextcloud server and all the favicons are getting generated automatically now. The HTML source code reveals the following url for the generated favicon: /apps/theming/favicon/contacts?v=0.

EDIT: typo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid favicon
3 participants