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

Update launcher icon with flat long-shadow variant #638

Closed
wants to merge 6 commits into from

Conversation

lokesh-krishna
Copy link
Contributor

As discussed at #317, here's the PR to update the app launcher icon. Round icon support hasn't been added and neither has support for adaptive icons. I don't know how to do either and I think we'll need Michael Cook's help for the latter with regards to a foreground and background layer unless someone here can do it.

All the .pngs were generated using the provided .svg.

Also, as mentioned at #595, the icons need go into the mipmap directory only if we are building different apks for different display density (which I think we aren't).

Also, the discussion at #595 mentioned the welcome icons but I don't think they need changing because they represent the service while these icons represent one specific product based off that service. Also, they look better in that context. That's just my understanding of it and I will make a PR for them as well if someone feels that is required.

Someone will need to credit Michael Cook for the icon in the About screen so that the requisite credits are present when this icon goes live. This is how he said he would like to be credited:

If you'd like to include credits, please do something like: Michael Cook, http://cookicons.co

Would be nice if this was also mentioned in the changelog for the version in which this goes live for that's how I discovered him in the first place and I'd like to pay that forward.

@di72nn
Copy link
Member

di72nn commented Oct 29, 2017

Can someone compare the app icon size to other app's icon sizes? If I understand correctly, icons should have some padding (here under the "Content area" caption).

See #645 for comparison.

@lokesh-krishna
Copy link
Contributor Author

Finally looked into the link and my understanding of it is that the recommendation is regarding system icons. The padding is to protect the icon from getting cut off by elements such as sidebars, which is obviously not a problem for launcher icons.

Am I missing something?

@di72nn
Copy link
Member

di72nn commented Dec 10, 2017

Yeah, it is about system icons, I didn't pay enough attention. I still have the impression that some padding is implied for launcher icons, but I can't find any numbers.

@lokesh-krishna
Copy link
Contributor Author

lokesh-krishna commented Dec 11, 2017

Have you tested the icon? It would be nice to see what it looks like in action and see if it is fine the way it is now.

I can't build apps on my current setup so it would be really nice if someone could and share some screenshots.

Seeing how the icon was made by someone who has made icons for plenty of others apps too, I think any requirements such as padding would already have been taken care of.

@lokesh-krishna
Copy link
Contributor Author

Sorry about bumping but can someone please test the icons so that this can be merged?

Copy link
Contributor

@Strubbl Strubbl left a comment

Choose a reason for hiding this comment

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

With this PR, the update of the icon, we need a several more changes.

  • Update of the Readme: icon needs to be changed to the new one and in the About section there shall be a paragraph crediting @themichaelcook
  • we need an icon, which looks the same, but indicates the debug version installed on the phone. Currently we have this in https://github.com/wallabag/android-app/tree/master/app/src/debug/res/drawable-xxxhdpi Without that icon i cannot test it du to having the release version installed for production purpose and not for testing
  • it would be nice if this PR has the credits for the about screen

@Strubbl
Copy link
Contributor

Strubbl commented Jan 29, 2018

This PR conflicts with #645

@lokesh-krishna
Copy link
Contributor Author

lokesh-krishna commented Feb 1, 2018

I've already updated the icon in #596. I'll add credits for the icon as well.

I checked the debug icon and if you could provide me with a SVG of the same then I could move the bug to the new icon and update that as well.

Making changes to the About screen is beyond my skill set presently. 🙈

As for the conflict with #645, that PR was opened to go with a version of this icon with added padding but I think I and @di72nn have arrived at an understanding regarding that. @di72nn, do you think it would be okay to close your PR?

@Strubbl
Copy link
Contributor

Strubbl commented Feb 1, 2018

Sorry, i forgot about #596. Thanks for the reminder. Thanks for adding credits to that PR.

The bug being used is the following unicode char: https://www.fileformat.info/info/unicode/char/1f41b/index.htm
You can also get the SVG from that site.

Currently, in the app we have navigation pane, which we can open from the left. There is also the official wallabag logo being used. But this logo we do not want to replace, don't we?
We only want the Android app logo to be changed, correct?

If we have all those changes in here and #596, from my point of view, we can go ahead merging this.
For the about screen i am going to open up an issue for adding the graphics credits.

@lokesh-krishna
Copy link
Contributor Author

Added a debug icon and added credits for the icon in #596.

Copy link
Contributor

@Strubbl Strubbl left a comment

Choose a reason for hiding this comment

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

LGTM

If @di72nn has no issues with this PR and #596, from my point of view we can merge both now.

@lokesh-krishna
Copy link
Contributor Author

Sorry about bumping but can we please move ahead with this?

@lokesh-krishna
Copy link
Contributor Author

I have figured out how to add support for adaptive icons and would like to open a new PR with adaptive icon support. I hope it is okay to close this one.

@Strubbl
Copy link
Contributor

Strubbl commented May 21, 2018

Yes, no problem @lokesh-krishna . Thanks a lot for your ongoing commitment and contribution to the project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants