Skip to content

Favicons for web UI - #1008

Merged
peternewman merged 21 commits into
OpenLightingProject:masterfrom
dandaka:favicons
Jan 30, 2016
Merged

Favicons for web UI#1008
peternewman merged 21 commits into
OpenLightingProject:masterfrom
dandaka:favicons

Conversation

@dandaka

@dandaka dandaka commented Jan 10, 2016

Copy link
Copy Markdown
Member

I have added favicons and changed index.html. But in my local olad I can't start webserver (Angular?) to start serving these files. It is serving all old files (like logo.png), but won't serve new once. Since all files are minified and there is no documentation, I thought it would be much easier for former developers to include these files.

@daveol please have a look

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Frontend?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually doesn't this want to be OLA

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Open Lighting Project" I think, assuming this shows up in FF etc

@peternewman

Copy link
Copy Markdown
Member

Thanks for doing this @dandaka ! I remember looking into it a year or so ago and then getting distracted, but it looks a lot easier now.

@dandaka

dandaka commented Jan 10, 2016

Copy link
Copy Markdown
Member Author

I have fixed references to other projects. Please have a look.

@peternewman peternewman added this to the 0.11.0 milestone Jan 10, 2016
@dandaka

dandaka commented Jan 10, 2016

Copy link
Copy Markdown
Member Author

Why don't my files serve from webserver? Angular settings?

@peternewman

Copy link
Copy Markdown
Member

The webserver is part of olad, so you'll need to do a full autoreconf/configure/make, then run olad/olad and browse to port 9090. You can probably fudge this given you don't necessarily care about the actual OLA stuff by just serving up olad/www from a webserver.

You'll also need to add all the new files to olad/www/Makefile.mk or equivalent as appropriate, depending on their location.

Comment thread olad/www/new/index.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think judging on what we've done elsewhere in the file, these may need to be fully qualified with /new/ on the start. It would be good to be consistent anyway.

@peternewman

Copy link
Copy Markdown
Member

Re-reading your comment about olad, it sounds like you are running it up, did you also have the deb installed? If so you'll probably need to use the --http-data-dir option to serve it up from your WIP folder:
http://docs.openlighting.org/ola/man/man1/olad.1.html

There is some documentation for the new UI here:
https://github.com/OpenLightingProject/ola/blob/master/javascript/new-src/README.md

We've also got Grunt and all that magic, so you could add the relevant stuff in there to allow us to regenerate the favicons if we change our logo, it's probably worth chatting to @daveol about that:
https://github.com/OpenLightingProject/ola/tree/master/javascript/new-src

We've also got the RDM test server, if you fancy adding favicons to that too:
https://github.com/OpenLightingProject/ola/tree/master/tools/rdm

And the RDM website :) :
https://github.com/OpenLightingProject/rdm-app
http://rdm.openlighting.org/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OLA please

@peternewman

Copy link
Copy Markdown
Member

Which original image file did you use BTW, was it from https://github.com/OpenLightingProject/logos ?

@dandaka

dandaka commented Jan 10, 2016

Copy link
Copy Markdown
Member Author

I've fixed descriptions. I need clarification on your comment "Can't we keep these, or is it not worth it?". Got this.

When you accept the pull request, I'll squash all this into a commit.

Yes, I have used original logo from your repo.

I was using gulp to generate these images. I think it will be too much for me now to set up a grunt task for possible future updates.

I can post to https://github.com/OpenLightingProject/logos image, that I have used to generate favicons. And all favicons. So anyone could fork and use in other repositories.

@dandaka

dandaka commented Jan 10, 2016

Copy link
Copy Markdown
Member Author

BTW, I have added @2x version of logo in new web UI. And fixed relative path according to your guide /new/img/ (it was different). Tested favicons in two browsers, seems to be working.

I'm running a simple webserver (from gulp) as you advised.

@peternewman

Copy link
Copy Markdown
Member

Thanks for putting Twitter and Opengraph back.

You needn't bother squashing, I regularly commit stuff unsquashed, in some cases with far more commits.

Perhaps just add a note to https://github.com/OpenLightingProject/ola/blob/master/javascript/new-src/README.md mentioning which tools you used, and a link to which source image. Sorry, I misread/assumed gulp and grunt were compatible, or a sub or superset of each other. Would adding a gulp task just be a case of copying your boilerplate files? Do you fancy doing that instead?

Did you not just use this image to make the favicon, or was it not big enough:
https://github.com/OpenLightingProject/logos/blob/master/raster/OLA%20Logo%20O%20Only.png

I don't think we need every single favicon in the logos repo, as the number of those will just keep growing with new hardware, OSes and software, as long as we can generate them from an original.

Did you give up trying to serve them via olad then?

@peternewman

Copy link
Copy Markdown
Member

Regarding the 2x, that's great, I think that was what I was looking at ages ago, rather than favicons. Is there any reason you can't use our original logo-mini.png and this for the 2x? https://github.com/OpenLightingProject/logos/blob/master/web%20fitted/OLA-Logo-Fitted-22px%402x.png

It's just we've then got the source Photoshop file too.

@peternewman

Copy link
Copy Markdown
Member

The 2x stuff looks simple enough I might try updating our old UI too (unless you fancy doing the honours :) ) https://github.com/OpenLightingProject/ola/tree/master/olad/www .

@peternewman peternewman self-assigned this Jan 10, 2016
@dandaka

dandaka commented Jan 11, 2016

Copy link
Copy Markdown
Member Author

Sorry for that, fixed.

Comment thread olad/www/Makefile.mk Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you want to keep the favicons directory you will have to specify a new variable at the top, something like:

favicondir = $(www_datadir)/new/img/favicons

and the at the begin of the files that you want to end up in the favicons dir you would have to specify:

dist_favicon_DATA = \

otherwise autotools will just end up putting them in the /new/img directory

@peternewman

Copy link
Copy Markdown
Member

You're almost there, you need to add the new content types to include/ola/http/HTTPServer.h too, which should fix the remaining Travis issues.

@peternewman

Copy link
Copy Markdown
Member

Congratulations @dandaka you're changes are now passing all the tests (it just happens that flake8 has had pep8 updated, which has caused some existing code untouched by you to break). I'm just working on fixing that in #1014 so if you update when it's merged your tests should go green.

There are just a few other outstanding comments, e.g. #1008 (comment)

Thanks again for all your work on this; it gets easier after the first bit! :)

@dandaka

dandaka commented Jan 14, 2016

Copy link
Copy Markdown
Member Author

Is there any reason you can't use our original logo-mini.png and this for the 2x?

  1. I have removed empty pixel from original logo-mini.png, no need for that
  2. OLA-Logo-Fitted-22px%402x.png has empty pixels too, which is a bad practice IMO

Since a single pic is not a big deal, I have replaced them with better options. But you can always stick to old files, I just don't see any reason to do that.

If I have missed some other comments, please point me to them.

@peternewman

Copy link
Copy Markdown
Member

Can you resync please @dandaka and the checks should go green.

@dandaka

dandaka commented Jan 24, 2016

Copy link
Copy Markdown
Member Author

Did that.

@peternewman

Copy link
Copy Markdown
Member

Thanks @dandaka . This all looks great. Feel free to add yourself to here if you want a credit:
https://github.com/OpenLightingProject/ola/blob/master/AUTHORS

Understood about the empty pixels. Do you have Photoshop? It would be superb if https://github.com/OpenLightingProject/logos/tree/master/web%20fitted could be updated so what's available there matches what's in the Web UI.

The comment/request about favicons and 2x for the other interfaces would still be very welcome (as well as some other stuff in the existing UI that could be 2x, but I guess we'd need vector images for them).

@nomis52 or @daveol any final comments before we merge?

Comment thread olad/OladHTTPServer.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://en.wikipedia.org/wiki/ICO_%28file_format%29#MIME_type and http://stackoverflow.com/questions/13827325/correct-mime-type-for-favicon-ico suggest a MIME type of image/x-icon should be correct, would you mind changing to that please @dandaka ?

@dandaka

dandaka commented Jan 26, 2016

Copy link
Copy Markdown
Member Author

@peternewman Fixed issues with MIME.

I consider fixing other icons with @2x a minor bug, which is not bothering right now.

You are welcome to open an issue for me in https://github.com/OpenLightingProject/logos/ project.

@peternewman

Copy link
Copy Markdown
Member

Thanks @dandaka . Issue raised here, I'll assign it when you accept the invitation:
OpenLightingProject/logos#7

Understood about the other bits; I'm being cheeky really. Although TBH I didn't realise quite how easy it is to fix.

@peternewman

Copy link
Copy Markdown
Member

You'll need to resync again please @dandaka .

@nomis52

nomis52 commented Jan 29, 2016

Copy link
Copy Markdown
Member

LGTM. Lets merge this.

@peternewman

Copy link
Copy Markdown
Member

Great. @dandaka can you resync for a final time, then @ mention nomis52 and I and someone will hit the merge button.

@dandaka

dandaka commented Jan 30, 2016

Copy link
Copy Markdown
Member Author

@nomis52 @peternewman Ready to merge.

peternewman added a commit that referenced this pull request Jan 30, 2016
@peternewman
peternewman merged commit 5ddf7e8 into OpenLightingProject:master Jan 30, 2016
@peternewman

Copy link
Copy Markdown
Member

Thanks @dandaka !

@dandaka

dandaka commented Jan 30, 2016

Copy link
Copy Markdown
Member Author

Hurray, glad to contribute!

@dandaka
dandaka deleted the favicons branch January 30, 2016 12:47
@peternewman

Copy link
Copy Markdown
Member

The question is what's next @dandaka ! :)

@peternewman peternewman mentioned this pull request Jul 12, 2026
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