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

Use vector version of openHAB logo #1027

Merged
merged 8 commits into from
May 10, 2021
Merged

Use vector version of openHAB logo #1027

merged 8 commits into from
May 10, 2021

Conversation

ecdye
Copy link
Contributor

@ecdye ecdye commented Apr 28, 2021

Signed-off-by: Ethan Dye mrtops03@gmail.com

@ecdye ecdye requested review from ghys and a team as code owners April 28, 2021 22:48
@hubsif
Copy link
Contributor

hubsif commented Apr 29, 2021

Generally, a nice improvement 👍
Though it doesn't work for me, e.g. it doesn't show the logo on the top left anymore. First guess is that the svg doesn't have a fixed size and thus doesn't get scaled properly. Did it work for you?

@ecdye
Copy link
Contributor Author

ecdye commented Apr 29, 2021

It does display for me but not fully the way it should, I'll work on playing with the settings for the css and stuff and get it fixed for all devices.

@hubsif
Copy link
Contributor

hubsif commented Apr 30, 2021

Hi @ecdye,
Looking good for the main site for me now!

I think it still needs some finetuning, though, e.g. the logo is huge on the HABot site.
Also, some sites are still using the non-vector "icons" (e.g. the HABot chat, about site, etc.). In my opinion we should use your good work as an opportunity to replace them all. Would it be possible for you to do that?

Note: Just making suggestions here, as I'm not in the position to make final decisions, that's up to @ghys.

@ecdye
Copy link
Contributor Author

ecdye commented Apr 30, 2021

Oh I had forgotten to check on the HABbot site thanks for the reminder. A lot of those little icons were favicons I don't know how well it will work to replace them as not all web browsers currently support vector favicons, any thoughts on that @ghys

@hubsif
Copy link
Contributor

hubsif commented Apr 30, 2021

Oh I had forgotten to check on the HABbot site thanks for the reminder. A lot of those little icons were favicons I don't know how well it will work to replace them as not all web browsers currently support vector favicons, any thoughts on that @ghys

I'd keep the favicons untouched since svg support is quite new. But I'd replace them by svg where else they have been used for convenience, e.g. the about site.

@ecdye
Copy link
Contributor Author

ecdye commented Apr 30, 2021

Well, I actually don't use HABot so I think Ill actually save that for another day when I can properly test the UI and make sure it looks right. Why don't we just start with the mainUI for now.

@ecdye ecdye force-pushed the img branch 2 times, most recently from 542a086 to 53037e7 Compare April 30, 2021 18:41
@hubsif
Copy link
Contributor

hubsif commented Apr 30, 2021

Nice! And one 👍 extra for the svg favicon and keeping the png as fallback!

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Nice initiative! I'll test it next week but this should make the logo crisper on retina displays and the like 👍
Why remove the icons which are referenced in the web app manifest though? https://github.com/openhab/openhab-webui/pull/1027/files#diff-126354c0cac3eaa37186ee3abdc3db5810ca6cb4e5f71061f1f87cf77a15533b

<?xml version="1.0" encoding="utf-8"?>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 170 37">
<style>
@media (prefers-color-scheme: dark) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite right because the user can override the preferred color scheme in the help & about page.
When the dark mode is active though, the root html element will have the theme-dark CSS class applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I’d skip that and stick to the „coloured“ version. I think it looks much better than the white in any case, including the favicon.
E92A9593-0BA1-4AA4-904B-ECF9F54A059C

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm okay with either. Which would you prefer @ghys?

Copy link
Member

@ghys ghys May 1, 2021

Choose a reason for hiding this comment

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

I made a white only image deliberately because IMO the contrast ratio of the gray circle/"HAB" against a dark background is poor, especially compared to the orange parts which stand out as a result.
As for the favicon, in the png version the logo is inside a white circle.

I don't think it looks horrible either way but I'd stick to the white version in dark mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super familiar with this web system, any tips on the right css for the override on the svgs icons?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's a straightforward solution that makes it possible to use inline CSS styling like the current prefers-color-scheme: dark.
I think for this to work the SVG has to be loaded inline, which would require using another webpack loader.
So, it's probably best to stick to two images.

@ghys
Copy link
Member

ghys commented Apr 30, 2021

Also, some sites are still using the non-vector "icons" (e.g. the HABot chat, about site, etc.). In my opinion we should use your good work as an opportunity to replace them all. Would it be possible for you to do that?

You can probably focus on the main UI for now and update HABot later if you wish. I'm not sure about the specs for favicons and the like but every time there's a equivalent vector icon available it should in general be used - depending on browser support but I think they all have good support these days.

@ecdye
Copy link
Contributor Author

ecdye commented May 1, 2021

Support for vector icons os good on all except Safari according to my tests.

@ecdye
Copy link
Contributor Author

ecdye commented May 1, 2021

Why remove the icons which are referenced in the web app manifest though? https://github.com/openhab/openhab-webui/pull/1027/files#diff-126354c0cac3eaa37186ee3abdc3db5810ca6cb4e5f71061f1f87cf77a15533b

I didn't remove them, I just minified and updated them to be consistent with the other icons.

@ghys
Copy link
Member

ghys commented May 1, 2021

My bad, I was confused by the red sizes and thought they were gone 🤦
However as stated above the white background circle was included on purpose to improve contrast, and you don't want a fully transparent logo because it doesn't stand out enough. So it needs to stay.

@ecdye
Copy link
Contributor Author

ecdye commented May 5, 2021

@ghys Any chance for a review, I think I have addressed all your concerns.

@ghys
Copy link
Member

ghys commented May 9, 2021

Thanks, almost all :)

The favicon should still have the white background as before for the same reasons - you won't see the gray circle (or barely) if the background is also a shade a gray:

Before/after:

image

The version on the left still looks better IMO.

And - more anecdotal - in the about page it's also fine to use a version of the logo with unaltered colors and a white background, instead of a monochrome version (and keep it pushed to the right to conserve vertical space):

Before
image

After
image

The logo on the top-left does look good and crisp though, nice! 👍

ecdye added 7 commits May 9, 2021 12:22
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
This also implements vector favicons for browsers that support it,
support for browsers that do not allow vector favicons has not changed.

Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
Signed-off-by: Ethan Dye <mrtops03@gmail.com>
@ecdye
Copy link
Contributor Author

ecdye commented May 9, 2021

@ghys I think I got them all, does this look good?

@relativeci
Copy link

relativeci bot commented May 9, 2021

Job #107: Bundle Size — 10.43MB (-0.48%).

5152292 vs 8b8a4eb

Changed metrics (4/8)
Metric Current Baseline
Initial JS 1.6MB(-1.13%) 1.61MB
Cache Invalidation 22.79% 22.55%
Modules 1455(+0.14%) 1453
Assets 239(-5.91%) 254
Changed assets by type (3/7)
            Current     Baseline
HTML 1.11KB (+9.86%) 1.01KB
IMG 178.08KB (-17.22%) 215.12KB
JS 8.14MB (-0.18%) 8.15MB

View Job #107 report on app.relative-ci.com

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Absolutely, thank you very much!

@ghys ghys merged commit 19395df into openhab:main May 10, 2021
@ghys ghys added this to the 3.1 milestone May 30, 2021
@ghys ghys added main ui Main UI enhancement New feature or request labels May 30, 2021
@mstormi
Copy link

mstormi commented Sep 21, 2021

Thanks for this!
Ethan is there any chance for you to add another improvement and make the logo a configurable file ?
So that a user can put up a svg or png file of his own ?

@ecdye
Copy link
Contributor Author

ecdye commented Sep 21, 2021

Not very easily to my knowledge that would be a better project for the webui maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants