Skip to content
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

Merged
merged 10 commits into from
Aug 7, 2023

Conversation

Nkiriobasi
Copy link
Contributor

@Nkiriobasi Nkiriobasi commented Aug 1, 2023

Closes #{issue number}

Motivation and context

Screenshots:

Before After
Paste screenshot Paste screenshot

Testing

Steps to test

Affected urls

Environment

New environment variables:

  • NEW_ENV_VAR: env var details

New or updated dependencies:

Dependency name Previous version Updated version Details
dependency/name v1.0.0 v2.0.0

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

✅ Tests will run for this PR. Once they succeed it can be merged.

Copy link
Member

@sashko9807 sashko9807 left a 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',
Copy link
Member

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'
Copy link
Member

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 }}
Copy link
Member

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} />
Copy link
Member

@sashko9807 sashko9807 Aug 2, 2023

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} />
Copy link
Member

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' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be reverted.

@Nkiriobasi
Copy link
Contributor Author

Alright, no problem

@ani-kalpachka
Copy link
Member

@Nkiriobasi thanks for the contribution!
I think we should keep the objectFit: 'cover' because without it the styles do not look appropriate.

@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Aug 4, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Aug 4, 2023
@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Aug 6, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Aug 6, 2023
Copy link
Contributor

@igoychev igoychev left a 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!

@igoychev igoychev merged commit 9521863 into podkrepi-bg:master Aug 7, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants