-
Notifications
You must be signed in to change notification settings - Fork 31
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
Touch and other icons #585
base: staging
Are you sure you want to change the base?
Touch and other icons #585
Conversation
@philli-philip is attempting to deploy a commit to the Technologiestiftung Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @philli-philip, thanks for the contribution! I will review the changes as soon as possible. Stay tuned, I'll get back to this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @philli-philip, thanks again for the contribution. I've got a few questions that need clarification. Also, the design of the monochrome icons doesn't really match the feel of the color logo, see my comment below.
While it certainly makes sense to include these optimizations for different browsers/devices, I'm a bit hesitant of merging them without being able to preview them. Do you know of any way to preview them without merging? Or perhaps you could point me to a good resource that explains the usage of such assets?
<browserconfig> | ||
<msapplication> | ||
<tile> | ||
<square150x150logo src="/images/mstile-150x150.png"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the mstiles. How does it work that here it's only one of the images referenced, but you actually added 5 different sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I'll link them.
public/images/safari-pinned-tab.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the filled logo doesn't really match the feel of the original logo. If only one color is permitted here (and for the white ones), it would make sense to leave the outlines of the original logo and remove the circular, light-green background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try that as I would agree that this it is the closest to the brand. But for readability I then decided that a filled version of the logo would be more suitable. Here the comparison:
Apple Touch Bar and Pinned Tab with outlined Logo Apple Touch Bar and Pinned Tab with filled Logo Microsoft Tiles with filled Logo Microsoft Tiles with outlined LogoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for the very small iOS use cases we go with the solid version and for MS-Tiles we go with the outlined version as the icons are displayed in large enough size so the details of the icon are well visible.
src/components/MapLayout.tsx
Outdated
href='/images/safari-pinned-tab.svg' | ||
color='#5bbad5' | ||
/> | ||
<link rel='shortcut icon' href='/images/favicon.ico' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this duplicating the definition in line 47?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Row 40 to 42 add the icon for the Safari Pinned Tabs, but yes row 43 is duplicated. I will remove it.
To explain how to test the assets let's start with the list of places I considered icons for:
So what needs to be done to test 100%?
This works not only with the production version but also with [Preview](Visit Preview) from Vercel in this case. If needed I can add a couple of screenshots verifying that the icons work. |
Thanks for you patience and contribution @philli-philip. I spoke with @dnsos and we agreed once more that the logo should be inline with the overall look and feel. I tested the following browsers / devices:
So in turn it would be necessary to solely use either the "outlined logo" like you suggested or try to use the original logo if possible. We don't use any blue color in within the CI. Maybe using the light green might also work? I wasn't able to test:
but I suggest the same to either use solely the outlined logo or stick to the original one. Let me know what you think. |
This pull request adds the assets and the linking needed to for better Fav icons, in particular: