Help #12

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants

No description provided.

Member

sferik commented Jul 24, 2012

Thank you for submitting this pull request. I looks much better than your earlier pull requests. I ❤️ the new logo you've created. I just have a few questions:

  1. The new logo image is larger than the existing logo image. What is the significance of the current size (884x288)? The width is approximately (but not exactly) 4 times the display width of 212px. Were you attempting to make an image suitable for a Retina display? If so, 2X should be sufficient. By stretching it from the original size, the image actually loses sharpness.
  2. The new logo image is almost 1 MB in size, compared to just 2.3 KB for the current logo. Even the large version of the logo is just 7.9 KB. Can you compress this image?
  3. Is there any particular reason why you've created a new file instead of overwriting the existing logo? The assets are being stored in git, so it should be easy to revert to an earlier version. It is not necessary to store every iteration of the logo as a separate file.
  4. Would you be willing to update the favicon to reflect the new branding?
  5. Why is it necessary to specify 1px padding on the map? Is this in any way related to the logo change?
  6. If adding a new logo somehow requires modifying the map padding, why did you create a new #map selector instead of using the existing #map selector on line 178?
  7. Would you mind squashing these two commits (and any subsequent commits) into a single commit and writing a more descriptive commit message? The title and description of this pull request also leave much to be desired.

Thanks again for your work on this! I hope you will consider my questions above and revise your pull request accordingly.

Member

sferik commented Jul 24, 2012

By the way, here are some references for writing good commit messages:

Welcome!

What is the significance of the current size (884x288)?

test

Why is it necessary to specify 1px padding on the map?

Neccessary? It's in address to Opera compatibility.

Member

sferik commented Jul 24, 2012

Could that same sharpness be achieved with an image that's 424px wide? If for some reason you need a 4X image, that would be 848px, however this image is 884px, which seems unnecessarily large.

Member

sferik commented Jul 24, 2012

Neccessary? It's in address to Opera compatibility.

It would be helpful if you included that reason in the commit message.

Could that same sharpness be achieved with an image that's 424px wide?

I'm uncertain now.

Member

sferik commented Jul 24, 2012

I'm uncertain now.

Could you please try it and see? I'm pretty sure the resolution only needs to be doubled, not quadruped (or increased by a factor of 4 and 9/53). If there's some reason why it needs to be increased by a larger factor, please explain why.

app/assets/stylesheets/screen.css
@@ -178,6 +178,7 @@ a.btn {
#map {
height: 100%;
width: 100%;
+ padding: 1px
@sferik

sferik Jul 24, 2012

Member

Missing semicolon (while this isn't strictly necessary on the last element, this is the preferred style of the project).

Member

sferik commented Jul 24, 2012

Since the logo is scaled up from the original, the edges of the text look blurry to me.

screenshot

Could you please try it and see? I'm pretty sure the resolution only needs to be doubled, not quadruped (or increased by a >factor of 4 and 9/53). If there's some reason why it needs to be increased by a larger factor, please explain why.

I think that there will be an increase in pixelation as the rendering is scaled.

Member

sferik commented Jul 24, 2012

The following might be of note:

Started GET "/assets/logos/adopt-a-hydrant-snow-capped.png" for 127.0.0.1 at 2012-07-24 14:37:49 -0400
Served asset /logos/adopt-a-hydrant-snow-capped.png - 304 Not Modified (0ms)

I don't understand how that's relevant. Can you please explain?

Member

sferik commented Jul 24, 2012

The font of the logo is Alternate Gothic.

The font of the logo is Alternate Gothic.

I don't own that font.

Member

sferik commented Jul 24, 2012

I don't own that font.

It is available for free on the web if you search for it by name. You could also use the large image without increasing the size.

Andrew Carpenter added some commits Jul 25, 2012

@ahcarpenter ahcarpenter closed this Jun 7, 2016

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