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

Converted all Assests to .webp format #261

Open
wants to merge 5 commits into
base: staging
Choose a base branch
from

Conversation

Bibisha06
Copy link
Contributor

this pr converts the pictures used in the website to .webp format

the hardcoded pictures were replaced with their .web versions.
Implemented automatic webP conversion for images using "webpImages" function for the ones being retrieved from cloudinary.

the pr fixes issue #256

Copy link
Collaborator

@mohit-nagaraj mohit-nagaraj left a comment

Choose a reason for hiding this comment

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

Hey I have addressed few of the things. Could u pls take a look at them? Thanks

@@ -170,7 +171,7 @@ const Share: React.FC<ShareProps> = () => {
<div className="bg-gradient-to-tr from-gray-900 to-indigo-800 p-6 rounded-lg shadow-md text-white rounded-xl flex flex-col h-full">
<div className="flex justify-start mb-4 items-center">
<Image
src="https://img.icons8.com/?size=100&id=62856&format=png&color=000000"
src="https://img.icons8.com/?size=100&id=62856&format=webp&color=000000"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This wudnt work i believe? could u check this one... attach ss if possible

package.json Outdated
@@ -71,6 +70,7 @@
"eslint": "8.57.1",
"eslint-config-next": "14.2.13",
"postcss": "^8.4.32",
"tailwindcss": "^3.3.6"
"tailwindcss": "^3.3.6",
"typescript": "5.8.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this dependency changed

@@ -0,0 +1,8 @@
export const convertToWebP = (url: string): string => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you explain the need of this function? if we gave already converted it why do we need utility function. the better approach would be updating those details from db as well

});

// Apply the external `convertToWebP` function if required
webpUrl = convertToWebP(webpUrl); // This ensures the URL is properly formatted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary here?

return url.replace(/\.(jpg|jpeg|png)$/, ".webp"); // Convert local images
}
if (!url.includes("cloudinary.com")) return url; // If not Cloudinary, return as is
return url.replace("/upload/", "/upload/f_webp/"); // Modify Cloudinary URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we have this url replaced to f_webp?

img: "sihlogo.webp",
},
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are so many changes which went missing from original coud u take a look here

@@ -71,6 +74,15 @@ import { UploadApiResponse } from "cloudinary";
*/
export async function POST(request: Request): Promise<Response> {
try {
// Check if the Content-Type header is multipart/form-data
Copy link
Collaborator

Choose a reason for hiding this comment

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

could u also verify if this is working once :) thanks

if (!deletedLead) {
return NextResponse.json({ error: "Lead not found" }, { status: 404 });
}

// If there's an image URL, delete it from Cloudinary
if (deletedLead.imageURL) {
if (deletedLead.imageUrl) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the fields changed now? i dont get why this change was ncessary! .........same here as well!! Check if this is working

const newLead = new Leadsmodel({
id: leadID,
...leadData,
});

// If image URL is provided, upload it to Cloudinary and convert to WebP
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this change necessary? I dont see how this change from the previous codebase needed this extra logic for saving img

@@ -168,7 +190,23 @@ export async function PUT(request: Request) {
{ ...leadData },
{ new: true }
);


Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

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.

2 participants