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.

Sign up for GitHub

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

Jump to bottom

feat: add src type to CloudinaryLoaderCldOptions interface #552

Merged
merged 9 commits into from
Nov 1, 2024
Merged

feat: add src type to CloudinaryLoaderCldOptions interface #552

merged 9 commits into from
Nov 1, 2024

Conversation

vickywane
Copy link
Contributor

Description

This PR will add the src type to CloudinaryLoaderCldOptions interface to fix the ts error being thrown from the cloudinary-loader.ts file.

Fixes This will close #425

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Fix or improve the documentation
  • This change requires a documentation update

Checklist

  • I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
  • I have created an issue ticket for this PR
  • I have checked to ensure there aren't other open Pull Requests for the same update/change?
  • I have performed a self-review of my own code
  • I have run tests locally to ensure they all pass
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes needed to the documentation

@vercel Vercel
Copy link

vercel bot commented Oct 15, 2024

@vickywane is attempting to deploy a commit to the Cloudinary DevX Team on Vercel.

A member of the Team first needs to authorize it.

@colbyfayock
Copy link
Collaborator

hey @vickywane looks like this is failing

can you try to seei f you can reproduce the error locally by running pnpm build inside of the next-cloudinary directory?

image

@vickywane vickywane force-pushed the fix/cloudinary-loader-ts branch from 687b45e to d8ac9f6 Compare October 15, 2024 17:18
@vickywane
Copy link
Contributor Author

@colbyfayock I have fixed that error.

It was being thrown up in the CldImage component due to adjusting the src property.

@vercel Vercel
Copy link

vercel bot commented Oct 15, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
next-cloudinary ✅ Ready (Inspect) Visit Preview Nov 1, 2024 7:15pm

@colbyfayock
Copy link
Collaborator

the update ends up breaking all of the images. did you test this locally?

image

https://next-cloudinary-mnw6gnapu-cloudinarydevx.vercel.app/

@colbyfayock
Copy link
Collaborator

@vickywane hey any updates?

@vickywane vickywane force-pushed the fix/cloudinary-loader-ts branch from 1984f52 to c1648d0 Compare October 19, 2024 14:57
@vickywane
Copy link
Contributor Author

vickywane commented Oct 19, 2024 •
edited
Loading

@colbyfayock I looked through the code and saw that specifying a src property was causing the image URL to be overwritten when spread in L86 .

I'm doing a bit of typecasting in L190 which helps us to still maintain the expected type for the cloudinaryLoader function.

I checked the deploy preview link you specified and the images are not showing up, but that's not due to the previous src property error that was being thrown.

@vickywane
Copy link
Contributor Author

@colbyfayock Have you been able to take a look at this?

colbyfayock
colbyfayock reviewed Oct 22, 2024
View reviewed changes
next-cloudinary/src/components/CldImage/CldImage.tsx Outdated
{
...cldOptions,

...{
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colbyfayock I read through and tested again and I realized this spread here was not needed.

I was initially trying to merge the properties of the cldOptions object with a modified type into a single object, but that's not needed since I am typecasting down the line.

@colbyfayock
Copy link
Collaborator

i need to find some time to spend reviewing this. its difficult with the formatting changes and i'm not quite sure the fixes are fixing the root cause or a bandaid. will try to find some time this week

@vickywane
Copy link
Contributor Author

@colbyfayock I have removed the formatting to make this easier to review ( sorry that came from my code editor
:) )

the fixes are fixing the root cause or a bandaid

I would say this is a fine mix of a bandaid and a fix.

@vickywane vickywane requested a review from colbyfayock October 28, 2024 13:19
@colbyfayock
Copy link
Collaborator

thanks. im at a conference but will be back thursday and will try to review

@vickywane
Copy link
Contributor Author

@colbyfayock Okay.

Looking forward to it!

@colbyfayock colbyfayock changed the base branch from main to beta November 1, 2024 19:09
@colbyfayock colbyfayock merged commit 74e1b96 into cloudinary-community:beta Nov 1, 2024
4 checks passed
@colbyfayock
Copy link
Collaborator

hey @vickywane just merged in the beta branch from some work being done on refactoring some of the code, made a few of the changes moot, but still fixes the loader tweaks

thanks for fixing this all. going to merge it back into the beta branch to go out sometime soon from there

thanks for your contribution. this PR is eligible for free Cloudinary Hackatoberfest swag. Please send me an email at [email protected] with your name, GitHub username, and a link to the PR where I'll send further instructions.

Happy Hacktoberfest!

@colbyfayock
Copy link
Collaborator

@all-contributors please add @vickywane for code

@allcontributors allcontributors bot mentioned this pull request Nov 1, 2024
Merged
@allcontributors AllContributors
Copy link
Contributor

@colbyfayock

I've put up a pull request to add @vickywane! 🎉

github-actions bot pushed a commit that referenced this pull request Nov 1, 2024
@semantic-release-bot
chore(release): 7.0.0-beta.10 [skip ci]
2892d85 
# [7.0.0-beta.10](v7.0.0-beta.9...v7.0.0-beta.10) (2024-11-01)

### Bug Fixes

* add src type to CloudinaryLoaderCldOptions interface ([#552](#552)) ([74e1b96](74e1b96)), closes [#425](#425)
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2024

🎉 This PR is included in version 7.0.0-beta.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

colbyfayock pushed a commit that referenced this pull request Nov 1, 2024
@allcontributors
docs: add vickywane as a contributor for code (#564)
8440f64 
Adds @vickywane as a contributor for code.

This was requested by colbyfayock [in this
comment](#552 (comment))

[skip ci]

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colbyfayock colbyfayock Awaiting requested review from colbyfayock

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] @ts-ignore in cloudinary-loader.ts
2 participants
@vickywane @colbyfayock