-
Notifications
You must be signed in to change notification settings - Fork 88
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
removed legacy objectFit property from next/image component #1534
removed legacy objectFit property from next/image component #1534
Conversation
✅ Tests will run for this PR. Once they succeed it can be merged. |
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.
Please keep in mind that, objectFit is a valid CSS property. What the warning from issue #1529 refers to is move the objectFit, inside the style of the image component, rather than it being a prop.
So if we have <Image ..... objectFit='contain'/>
We need to move objectFit into style prop e.g.
<Image style={{objectFit: 'contain}} />
@@ -15,7 +15,6 @@ export const AboutHeading = styled(Typography)(() => ({ | |||
export const Avatar = styled(Image)(() => ({ | |||
borderRadius: '50%', | |||
textAlign: 'center', | |||
objectFit: 'cover', |
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.
Should be reverted.
@@ -10,14 +10,14 @@ const defaultImage = { | |||
type Props = { | |||
height: string | |||
src?: string | null | |||
objectFit?: 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' | |||
// objectFit?: 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' |
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.
Should be reverted
@@ -29,7 +29,7 @@ export default function FeaturedImage({ | |||
<Image | |||
fill | |||
sizes="100vw" | |||
style={{ objectFit, objectPosition: defaultImage.objectPosition }} | |||
style={{ objectPosition: defaultImage.objectPosition }} |
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.
Should be reverted
@@ -82,7 +82,7 @@ export default function CampaignSlider({ sliderImages }: Props) { | |||
<Root className={classes.container}> | |||
<div style={{ textAlign: 'center' }}> | |||
{/* A11Y TODO: Add proper alt text*/} | |||
<Image src={sliderImages[0]} alt="campaign" height={300} width={400} objectFit="cover" /> | |||
<Image src={sliderImages[0]} alt="campaign" height={300} width={400} /> |
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 objectFit='cover'
inside the style prop
@@ -96,7 +96,7 @@ export default function CampaignSlider({ sliderImages }: Props) { | |||
{sliderImages.map((image, index) => ( | |||
<div key={index}> | |||
{/* A11Y TODO: Add proper alt text*/} | |||
<Image src={image} alt="campaign" height={300} width={400} objectFit="cover" /> | |||
<Image src={image} alt="campaign" height={300} width={400} /> |
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 objectFit='cover'
inside the style prop
@@ -21,7 +21,7 @@ export default function TeamMembersSection() { | |||
<Image | |||
alt="Team image" | |||
src={teamImagePath} | |||
style={{ maxWidth: '100%', height: 'auto', objectFit: 'contain' }} | |||
style={{ maxWidth: '100%', height: 'auto' }} |
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.
Should be reverted.
Alright, no problem |
@Nkiriobasi thanks for the contribution! |
…ge component" This reverts commit f3a3460.
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.
thanks for the help!
Closes #{issue number}
Motivation and context
Screenshots:
Testing
Steps to test
Affected urls
Environment
New environment variables:
NEW_ENV_VAR
: env var detailsNew or updated dependencies:
dependency/name
v1.0.0
v2.0.0