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 fallback_icon_color query parameter to API. #14

Merged
merged 4 commits into from
Oct 27, 2016
Merged

Add fallback_icon_color query parameter to API. #14

merged 4 commits into from
Oct 27, 2016

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Oct 22, 2016

If provided in the query parameters, this will use a hash of the URL passed in to color the letter icon used. The color is used when there is no icon of the requested size, and no "main color" could be found in smaller icons.

The idea is that this will prevent so many icons being grey when a large icon size is requested.

If provided in the query parameters, this will use a hash of the URL passed in to color the letter icon used. The color is used when there is no icon of the requested size, and no "main color" could be found in smaller icons.

The idea is that this will prevent so many icons being grey when a large icon size is requested.
@mat
Copy link
Owner

mat commented Oct 22, 2016

Hey @mmkal first of all thanks for your work!

While I find the idea interesting I don't believe it's the right approach to fix the (annoying) issue which is the grey icons: Having some background color for the sake of having a colorful icon is not really the goal of extracting the 'main color' from other existing icons. The goal is to resemble the actual favicon at least in terms of color. This is not at all the case with randomized colors.

That said, I definitely can see the value of setting another fallback color as grey - for users like you who prefer randomized colors or for users who want the fallback color be more fitting to the rest of their UI.

My proposal: Instead of a colorize_letters parameter we introduce a fallback_iconcolor parameter which would allow for that and which is even more flexible.

What do you think?

@mmkal
Copy link
Contributor Author

mmkal commented Oct 22, 2016

That's a great idea actually - I can just pass in my arbitrary colour as a query parameter, and if it doesn't get used because your colour finder was able to parse a smaller icon, so much the better. I could update this pull request to do that instead?

@mat
Copy link
Owner

mat commented Oct 23, 2016

Of course, please, give it a shot 😃👍

This allows clients to pass in a hex value as a fallback letter color.
(used ? when it should have been &)
@mmkal mmkal changed the title Add colorize_letters query flag to API. Add fallback_icon_color query parameter to API. Oct 27, 2016
@mat
Copy link
Owner

mat commented Oct 27, 2016

Thank you! From a quick view this looks almost exactly like how I would've done it 😀 But please give me the time to have a closer look on Sunday hopefully...

Please feel free to add yourself to the Contributors section in the Readme in the meantime.

redirectPath := lettericon.IconPath(letter, size, iconColor)

Copy link
Owner

Choose a reason for hiding this comment

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

pls remove

@@ -28,6 +28,7 @@ url | http://yelp.com | | required
size | 120 | Desired **minimum** icon size | required
formats | png,ico | Comma-separated list of accepted image formats: png, ico, gif | `png,ico,gif`
fallback\_icon\_url | *HTTP image URL* | If provided, a redirect to this image will be returned in case no suitable icon could be found. This overrides the default fallback image behaviour. |
fallback\_icon\_color | ff0000 | If provided, letter icons will be colored with the hex value provided, rather than be grey, when no color can be found for any icon.
Copy link
Owner

Choose a reason for hiding this comment

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

Is ff0000 really the actual grey in use? (I honestly don't know)

Copy link
Owner

@mat mat Oct 27, 2016

Choose a reason for hiding this comment

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

Ah, sorry, this is an example value, not the default. So, disregard.

Copy link
Contributor Author

@mmkal mmkal Oct 27, 2016

Choose a reason for hiding this comment

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

See below - ff0000 is bright red red (equivalent to rgb(255, 0, 0)) so should make it visually pretty obvious what it does, especially when contrasted with the line above. Not sure what the deployment story is, but it should be easily visible if these changes go in and the server is updated.

@@ -38,6 +39,7 @@ fallback\_icon\_url | *HTTP image URL* | If provided, a redirect to th
|<https://icons.better-idea.org/icon?url=yelp.com&size=64>|![Icon for yelp.com](https://icons.better-idea.org/icon?url=yelp.com&size=64)|
|<https://icons.better-idea.org/icon?url=yelp.com>|size missing|
|<https://icons.better-idea.org/icon?url=httpbin.org/status/404&size=64>|![Icon for non-existent page](https://icons.better-idea.org/icon?url=httpbin.org/status/404&size=64)|
|<https://icons.better-idea.org/icon?url=httpbin.org/status/404&size=64&fallback_icon_color=ff0000>|![Icon for non-existent page](https://icons.better-idea.org/icon?url=httpbin.org/status/404&size=64&fallback_icon_color=ff0000)|
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use a nice, more colorful background color as example which is different from the default to make it even more obvious what this param is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I had the same thought - ff0000 is bright red!

@mat mat merged commit d61086d into mat:master Oct 27, 2016
@mat
Copy link
Owner

mat commented Oct 27, 2016

Thanks for your contribution @mmkal, much appreciated!

@mmkal
Copy link
Contributor Author

mmkal commented Oct 28, 2016

No problem - thank you for taking it in, I'm using it now and my page is looking great!

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

Successfully merging this pull request may close these issues.

2 participants