-
Notifications
You must be signed in to change notification settings - Fork 0
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
lil design updates #22
Conversation
Woohoo! new surge deployment available for viewing! 🎉 surgereviewrckikfumdytoilv.surge.sh |
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.
Couple comments/questions. We can remove bootstrap all the way later. 👍
src/components/Header.js
Outdated
<Container> | ||
<RibbonContainer | ||
color="white" | ||
backgroundColor="#E25E3E" |
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.
We should wrap this component in withTheme
and use props.theme.brand
for this color instead of hard coding.
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.
aye
src/components/RibbonContainer.js
Outdated
}, | ||
} | ||
const InnerContainer = ({ children, margin, padding }) => ( | ||
<g.Div |
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 prefer using g.div
in this case since it takes care of rendering the children and passing the props along, but this works I suppose
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 tried that, but it wasn't working for me... might've been something else I had wrong, will check again
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 tried readdressing this but couldn't figure out how to use g.div
and have it accept padding and margin as arguments
backgroundColor="#F9F9F9" | ||
padding={`${rhythm(2)} ${rhythm(3 / 4)}`} | ||
> | ||
<g.Img |
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 kind of liked the logo. Did we remove this for accessibility? If so we can do both. If you think it looks better, I'm ok with that.
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 felt like a lot having it twice so close together
No description provided.