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

Empty pixels removed and some cleanup #8

Merged
merged 3 commits into from
Jan 31, 2016

Conversation

dandaka
Copy link
Member

@dandaka dandaka commented Jan 30, 2016

As requested in #7

@nomis52
Copy link
Member

nomis52 commented Jan 31, 2016

LGTM.

If you could do an 'OLE' logo for Open Lighting Embedded that would be great. We don't have one:

https://www.openlighting.org/openlightingproject/projects/

nomis52 added a commit that referenced this pull request Jan 31, 2016
Empty pixels removed and some cleanup
@nomis52 nomis52 merged commit 89e0a71 into OpenLightingProject:master Jan 31, 2016
@peternewman
Copy link
Member

Sorry @dandaka can these be the same versions as used in the web UI. It looks like the web UI is W: 51px | H: 21px, but what you've uploaded is W: 54px | H: 22px . Or will the wider logo fit the UI?

@peternewman
Copy link
Member

We have an OLE logo @nomis52 it just looks rather rough:
https://github.com/OpenLightingProject/logos/pull/6/files

@dandaka dandaka deleted the empty-pixels branch January 31, 2016 16:38
@DouglasHeriot
Copy link
Contributor

So, the backstory. In the original web UI, the OLA logo is placed right up against the edge of the page, and I wanted to create a drop-in replacement for it without having to touch CSS and layout. Hence, the empty pixels exist to give it a little spacing. This matched roughly what the old file had in it.

I agree it’s nice to not have empty pixels, and they are unnecessary for new work.

However, the solution should have been to simply trim the empty pixels off, not to scale up the logo to fill them. I paid attention to making the logo align to pixel boundaries so it is clear at the small sizes, and not blurry. Hence the name of the directory "web fitted".

As seen in the attached screenshot, the new versions (top) disregard this, and have blurry edges everywhere.

Yeah, I know this is a bit pedantic, but I would like to fix this up, and will submit a pull request soon.

screen shot 2016-02-01 at 1 24 46 pm

@nomis52
Copy link
Member

nomis52 commented Feb 1, 2016

Ah yes, looking the logos now I can see the difference!

@peternewman
Copy link
Member

Thanks for the comments @DouglasHeriot . I certainly appreciate the attention to detail.

So to muddy the waters further, as per my comments above and the commit here:
dandaka/ola@020d091

Would I be thinking the OLA repo is currently correct for the new UI, which doesn't need the padding built in, because the image height has just shrunk, presumably by the removal of the blank row? It's the logo repo that's wrong.

I think ideally we want old UI 22px and 22px@2x (mobile) and 48px and 48px (normal) and new UI (no blank pixels) 22px and 22px@2x images. The RDM test server also uses the large image, but I don't know if it needs the blank pixels or not.

@dandaka
Copy link
Member Author

dandaka commented Feb 7, 2016

@DouglasHeriot Your version is clearly better, but requires manual tuning in all cases. I would be glad, if you fix this.

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.

4 participants