-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use styled components #30
base: master
Are you sure you want to change the base?
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.
I take it this PR is a draft, but nonetheless here's some feedback
const LogoImage = styled.img` | ||
border-radius: 0.25rem; | ||
width: 3rem; | ||
` |
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.
Let's put all local components below the exported. Priority should be:
- Metadata stuff (interfaces/types, config, or constants)
- Public stuff (exports)
- Private stuff (local stuff like components or utilities).
const LogoImage = styled.img` | ||
border-radius: 0.25rem; | ||
width: 3rem; | ||
` | ||
|
||
const Header = () => { |
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 like export const
instead of export default
. It's clearer and allows you to export types an other things that a module may want to export.
app/components/Header.tsx
Outdated
background: #1a1e23; | ||
border: 1px solid #1a1e23; |
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.
Why is a border needed if it matches the background color?
app/components/Header.tsx
Outdated
@@ -8,6 +8,34 @@ const LogoImage = styled.img` | |||
width: 3rem; | |||
` | |||
|
|||
const Anchor = styled.a` |
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.
Move below export. Also the name should be specific to the component's purpose. Isn't this a call-to-action button? Why not name it CtaButton
?
display: inline-flex; | ||
justifiy-content: center; | ||
justify-content: center; |
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.
Fixup the previous commit for the typo. Should we consider using JS instead of the string template in order for type checking to help us with typos?
${(props) => { | ||
const backgroundColor = props.ghost ? "transparent" : "#1a1e23" | ||
|
||
return css` | ||
background: ${backgroundColor}; | ||
border: 1px solid ${backgroundColor}; | ||
` | ||
}} | ||
|
||
&:hover { | ||
${(props) => { | ||
if (props.ghost) | ||
return css` | ||
background: color-mix(in srgb, white 5%, transparent 100%); | ||
border: 1px solid transparent; | ||
` | ||
else | ||
return css` | ||
background: #15181d; | ||
border: 1px solid #15181d; | ||
` | ||
}} | ||
} |
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.
This all becomes simpler with no border.
background: #15181d; | ||
border: 1px solid #15181d; | ||
background: color-mix(in srgb, white 5%, transparent 100%); | ||
border: 1px solid transparent; |
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.
Why the border is not using the same color function for its tweening? But perhaps the border should be removed anyway, but if not then this question is relevant.
|
||
const LogoImage = styled.img` | ||
border-radius: 0.25rem; | ||
width: 3rem; | ||
` | ||
|
||
const Anchor = styled.a` | ||
const Anchor = styled.a<{ | ||
ghost?: boolean |
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.
ghost
isn't used. Why is it added in this commit?
@@ -52,6 +52,54 @@ const Anchor = styled.a<{ | |||
} | |||
` | |||
|
|||
const Button = styled.a<{ | |||
ghost?: boolean |
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.
If ghost
is used everywhere or nowhere, then why is it even a parameter?
No description provided.