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

Added icons and logos for Bosch SHC integration. #1164

Merged
merged 3 commits into from May 14, 2021

Conversation

tschamm
Copy link
Contributor

@tschamm tschamm commented Apr 11, 2020

Proposed change

New integration which introduces support for devices managed by the Bosch Smart Home Controller system. PR for home-assistant.io doc and home-assistant core repos are in preparation.

Type of change

  • Add a new logo or icon for a new integration
  • Add a missing icon or logo for an existing integration
  • Replace an existing icon or logo with a higher quality version
  • Removing an icon or logo

Additional information

Checklist

  • The added/replaced image(s) are PNG
  • Icon image size is 256x256px (icon.png)
  • hDPI icon image size is 512x512px for (icon@2x.png)
  • Logo image size has min 128px, but max 256px, on the shortest side (logo.png)
  • hDPI logo image size has min 256px, but max 512px, on the shortest side (logo@2x.png)

@cogneato
Copy link
Contributor

I see where the logo source comes from, but I'm thinking the logo should just be the Bosch medallion and name? Let's get a second opinion from @frenck

logo

logo@2x

@cogneato cogneato added the in-progress This PR/Issue is currently being worked on label Apr 12, 2020
@frenck
Copy link
Member

frenck commented Apr 12, 2020

Yeah, I agree with you. We are looking for logo's, not product presentation images. 👍

@frenck frenck added the has-parent This PR has a parent PR in a other repo label Apr 12, 2020
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

@tschamm Thanks for updating the PR. Could you please elaborate on this choice? As @cogneato was so kind to deliver some high-quality images IMHO.

The logos you've currently added, are not transparent (which we prefer) and have a lot of white space around the actual logo.

@cogneato
Copy link
Contributor

@tschamm Yes, the two files I provided are sized and ready to go if you want to add them

@adrianmihalko
Copy link
Contributor

adrianmihalko commented Apr 12, 2020

Guys, Bosch logo is already present as high quality transparent logo and icon in bmp280. I suggest to remove this PR and open a new one by symlinking boschshc to bmp280. No need for duplicate images.

Other Bosch devices are symlinked too: bme280/, bme680/ ---> bmp280/

@frenck
Copy link
Member

frenck commented Apr 12, 2020

Thanks, @adrianmihalko absolutely correct 👍

@tschamm
Copy link
Contributor Author

tschamm commented Apr 12, 2020

Thanks, good point!
For the logo I agree, even if Bosch changed their logo 2019 to a flat design. The one used in bmp280 is the old one. I suggest I update the PR with a symlink to bmp280 logo, and add a separate PR for updating to the new Bosch logo.
For the icon of the integration I prefer to use the Bosch Smart Home icon I added in this PR.

@adrianmihalko
Copy link
Contributor

adrianmihalko commented Apr 12, 2020

Thanks, good point!
For the logo I agree, even if Bosch changed their logo 2019 to a flat design. The one used in bmp280 is the old one. I suggest I update the PR with a symlink to bmp280 logo, and add a separate PR for updating to the new Bosch logo.
For the icon of the integration I prefer to use the Bosch Smart Home icon I added in this PR.

One more thing, logo is not high quality + has white space around it + not transparent:

Screenshot 2020-04-12 at 18 56 52

Please use this one, if you want flat logo:

https://worldvectorlogo.com/logo/bosch-logo-simple

Or classic:

https://worldvectorlogo.com/logo/bosch

@tschamm tschamm requested a review from frenck April 13, 2020 20:25
frenck
frenck previously approved these changes Apr 14, 2020
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@frenck frenck added awaits-parent Awaits the merge of an parent PR and removed in-progress This PR/Issue is currently being worked on labels Apr 14, 2020
@tschamm
Copy link
Contributor Author

tschamm commented Jan 25, 2021

The upstream HACS default PR fails without this being merged.
Link to HACS default PR: hacs/default#792

@frenck
Copy link
Member

frenck commented Jan 25, 2021

@tschamm This one cannot be merged against a custom integration, as this PR add graphics for a core intrgation.

@tschamm
Copy link
Contributor Author

tschamm commented Jan 26, 2021

You're totally right of course, I'm sorry. I'll add another PR for the custom integration.
I would like to leave this PR open, until the core integration is merged. It's still in review.

@frenck frenck merged commit c77c556 into home-assistant:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits-parent Awaits the merge of an parent PR has-parent This PR has a parent PR in a other repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants