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

Update Keyhole logos + use CDN hosted fonts + fix KeyholeLogo prop types error #11

Merged
merged 4 commits into from Apr 15, 2019

Conversation

superhussain
Copy link
Contributor

@superhussain superhussain commented Apr 15, 2019

Fixes #9
Fixes #10

Summary

  • update keyhole logo and icon links
  • use hosted version of tiempos and quicksand fonts
  • fix keyholelogo prop types error
  • add KeyholeLogo component to atoms

Screenshots

Before:
before
After:
after

Copy link
Contributor

@laijoann laijoann left a comment

Choose a reason for hiding this comment

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

Loooovely 🙌
One minor suggestion: if KeyholeLogo and HeaderLogo share the theme proptypes, perhaps one can import the other? Otherwise, good to merge!

@superhussain
Copy link
Contributor Author

Loooovely 🙌
One minor suggestion: if KeyholeLogo and HeaderLogo share the theme proptypes, perhaps one can import the other? Otherwise, good to merge!

KeyholeLogo and HeaderLogo don't necessarily share the same prop types. They're made to be independent of each other (possibly move KeyholeLogo into an atom?).

@laijoann
Copy link
Contributor

laijoann commented Apr 15, 2019

Loooovely 🙌
One minor suggestion: if KeyholeLogo and HeaderLogo share the theme proptypes, perhaps one can import the other? Otherwise, good to merge!

KeyholeLogo and HeaderLogo don't necessarily share the same prop types. They're made to be independent of each other (possibly move KeyholeLogo into an atom?).

KeyholeLogo and HeaderLogo don't need to share the same full set of proptypes! I'm referring specifically to the theme bit. E.g. you can import HeaderLogo.propTypes.theme into KeyholeLogo's theme proptype because HeaderLogo directly throws props.theme into KeyholeLogo's prop theme anyway. Alternatively, if KeyholeLogo's prop theme really only has 2 options then it shouldn't directly inherit props.theme from HeaderLogo.

And yes, KeyholeLogo would be good as an atom! 🐾

@laijoann laijoann merged commit 2b381ea into master Apr 15, 2019
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.

HeaderLogo's defaultProps conflicts with KeyholeLogo's proptypes Keyhole logo on custom theme is outdated
2 participants