-
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
Implement background image with gatsby-plugin-image #533
Implement background image with gatsby-plugin-image #533
Conversation
…le-web-app-capable"
…ad of gatsby-background-image
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.
Main change is here.
Visit the preview URL for this PR (updated for commit 66e4dcb): https://estuary-marketing--pr533-brenosalv-tech-debt-zo36fz6y.web.app (expires Thu, 14 Nov 2024 19:15:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 76f6b095a0752e5d9c6c890267f9fdc3e392161e |
className={clsx(backgroundImage, swoopingLines)} | ||
placeholder="blurred" | ||
quality={100} | ||
backgroundColor="#04192b" |
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.
Can we move the background colors into a shared file? That way we only have one spot we need to update color.
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.
It's a good point. I created a separate shared object for the colors.
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.
one small change that I honestly think can probably wait.
@travjenkins If it was added only for
|
<div className={fullWidth}> | ||
<StaticImage | ||
alt="" | ||
src="../../images/circles-background.png" | ||
className={clsx(backgroundImage, fullHeight)} | ||
placeholder="blurred" | ||
quality={100} | ||
/> | ||
<div className={className}>{children}</div> | ||
</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.
All of the BackgroundImages/*
components should probably share a single root component since they are all the same except for the src
setting.
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.
The StaticImage component does not accept to receive prop values as variables. They should be constants. So we can't create a separate component for it, but I created a separate component for the wrapper div
element.
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.
lgtm
#440
Changes
Tests / Screenshots
Docs
https://www.gatsbyjs.com/docs/how-to/images-and-media/using-gatsby-plugin-image/#background-images