-
Notifications
You must be signed in to change notification settings - Fork 65
Modify social icons to latest branding #323
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
Conversation
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.
Just thinking about timing. Do these make sense to be released in a minor version? How long will it be until we are ready to publish a major version? Seems like there are external dependencies which affect the major version timing? Should we ship these sooner?
src/js/icons/Mail.js
Outdated
@@ -4,7 +4,12 @@ import { StyledIcon } from '../StyledIcon'; | |||
|
|||
const Mail = forwardRef((props, ref) => ( |
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.
How does "Mail" fit the "social icon" theme? Less about a brand and more about the sharing mechanism?
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.
Good call, it does have a specific color on it when I do plain
-- so let me confirm with Chris if it was a specific app/company.
If it's just generic, I'd opt for it not having a specified HEX color and not be considered part of this "social icon" set.
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 connected with Chris. That "Mail" option is one we have for brand to use alongside other social icons in a "share to" kind of scenario. It is not a specific company icon, so from an open-source standpoint it doesn't necessarily make sense to be grouped with this.
It seems in some of his future revisions for icons, he's exploring a "filled" version of some of the icons so that might make more sense.
Given that discussion, I'm going to remove the modification to this icon for now.
Wasn't sure if shipping these in a minor version would break backwards compatibility semantic versioning rules. It's kind of a blurry line in the sense that they are modified and look different than old logos, but also it's just keeping up to date with external brands and it doesn't feel like we should have to major release each time some other brand changes its logo. Talking myself into including these in a minor release, but what do you think @halocline @jcfilben |
I'm comfortable with considering these updates as a minor release |
My thinking is:
So, I'd be okay with the minor. |
Great, have updated the base branch to |
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.
Looks good!
Update social icons to latest branding. Pointing this to "NEXT" branch to stage for next major release.
Tested locally in storybook:







