-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
chore(www): Refactor homepage-logo-banner
#25348
Conversation
not bored enough for responsive array things tho ;)
OK this is GTG 😅 |
Hey YouTuuuuube link missing |
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.
For this and the other PRs, I think it would be better to do instead something like:
const logoStyles = {
position: `relative`,
// other styles
}
export default function HomePageLogoBanner() {
return <div sx={logoStyles}>
{/* more stuff */}
</div>
}
As I said in another issue, making a component for each thing we want to style adds indirection and makes it harder to reason if html is semantic or not.
Yeah, thanks for calling it — I was kinda telling myself this would make the review easier, but … lemme fix this one now. |
display: none; | ||
} | ||
` | ||
const LogoGroup = props => ( |
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.
Btw. I feel like up to this object length I'd go inline without even thinking about it.
So … what are your thoughts re:"responsive arrays" @tesseralis?
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.
Could you leave more details on this? I'm not sure what "responsive arrays" are
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.
[null, null, null, "20px"]
— https://theme-ui.com/sx-prop/#responsive-values
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.
Ohhhhhhhh. Yeah they're great :P
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.
For me there are cases where I prefer to not use them, … and also the more null
s, the more torn I am :)
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.
Btw. did you see the v1 roadmap?
system-ui/theme-ui#832
<Title> | ||
<section | ||
sx={{ | ||
borderBottom: t => `1px solid ${t.colors.ui.border}`, |
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 think you can do:
borderBottom: t => `1px solid ${t.colors.ui.border}`, | |
borderBottom: t => `1px solid ${t.colors.ui.border}`, | |
borderBottom: 1, | |
borderColor: `ui.border`, |
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.
ha just pushed right after you merged — needed a borderStyle: "solid"
to make that work
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, there's one thing you can simplify with the border.
Switch from template literal/string styles to
sx
and object styles.