Skip to content

feat(nuxt-img): add custom slot for full control of rendering#1626

Merged
danielroe merged 18 commits into
nuxt:mainfrom
danekslama:feat/img-placeholder-slot
Jan 6, 2025
Merged

feat(nuxt-img): add custom slot for full control of rendering#1626
danielroe merged 18 commits into
nuxt:mainfrom
danekslama:feat/img-placeholder-slot

Conversation

@danekslama

@danekslama danekslama commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

🔗 Linked issue

resolves #1307

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR add a placeholder slot for NuxtImg component

Comment thread docs/content/2.usage/1.nuxt-img.md Outdated
Comment thread docs/content/2.usage/1.nuxt-img.md Outdated
@danekslama danekslama requested a review from Baroshem December 13, 2024 19:18
Comment thread src/runtime/components/NuxtImg.vue Outdated
@danekslama danekslama requested a review from danielroe December 19, 2024 10:48

@Baroshem Baroshem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work! :)

@danekslama danekslama requested a review from Baroshem December 19, 2024 13:00
@codecov-commenter

codecov-commenter commented Dec 20, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 62.78%. Comparing base (a2db479) to head (66a9a6b).

Files with missing lines Patch % Lines
src/runtime/components/NuxtImg.vue 66.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1626      +/-   ##
==========================================
- Coverage   62.87%   62.78%   -0.09%     
==========================================
  Files          79       79              
  Lines        3539     3547       +8     
  Branches      413      413              
==========================================
+ Hits         2225     2227       +2     
- Misses       1285     1291       +6     
  Partials       29       29              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread src/runtime/components/NuxtImg.vue
@danekslama danekslama requested a review from danielroe December 27, 2024 11:06
@danekslama

Copy link
Copy Markdown
Contributor Author

I added the custom prop which acts similarly to the NuxtLink custom prop. I also updated the docs to show how to create custom placeholders with it.

@danielroe danielroe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great - this is very promising!

Would you be able to add some tests? 🙏

@danekslama

Copy link
Copy Markdown
Contributor Author

I'll be honest, I'm fairly new to unit testing, but I have some experience with Cypress. I added a test to check if the placeholder functionality and attribute fallthrough work as expected when the custom prop is set, and it seems they do.

@danekslama danekslama requested a review from danielroe December 28, 2024 11:37
@danekslama

Copy link
Copy Markdown
Contributor Author

@danielroe is this okay or should I add more tests?

Comment thread src/runtime/components/NuxtImg.vue Outdated
...imgAttrs,
...attrs,
},
isLoaded: isImageLoaded,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this looks perfect now, just one change - I think let's not expose isImageLoaded as this can be misleading - on server load the load may take place before hydration and this will never end up being set to true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if it's significant, we could also open a new PR to implement the functionality - maybe through an approach similar to error handling?

@danielroe danielroe changed the title feat(placeholder): add placeholder slot feat(nuxt-img): add custom slot for full control of rendering Jan 6, 2025
@danielroe danielroe merged commit a1d459f into nuxt:main Jan 6, 2025
@github-actions github-actions Bot mentioned this pull request Jan 6, 2025
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.

Placeholder slot for <NuxtImg>

4 participants