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

glim. Final project by Izabel, Linda and Martin #56

Open
wants to merge 334 commits into
base: main
Choose a base branch
from

Conversation

Martin-Joensson
Copy link

Netlify link
https://glim-skincare.netlify.app/

Collaborators
[izzibizz, linda-f, martin-joensson]

Copy link

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Hi Izabel, Linda & Martin! Big congrats to completing your final project 🚀 🌕 🦩
I think we were all impressed by your project during the graduation demo but I want to say it again, that it looks so professional in so many ways. The design is on point,
I love the frosted header, the color scheme and image choices. And small little nice things, like the loading sparkles ✨ You also nailed the functionality of this app, and I'm impressed that you successfully implemented Stripe (thanks for sharing demo card details 😉). Your app is responsive and the UX is thought through. The code is mostly clean, but I left some comments on where I think you can benefit from refactoring.

Some other things that I think you can work on for further improvements and to make your app work even better:

  • After signing up it would be nice to stay logged in
  • Featured products carousel started in the end for me, so clicking right doesn’t do anything
  • Check spelling of your copy (will make it look more professional)
  • On the About page, it would be nice if the LinkedIn and GitHub were icons like in the footer, or at least that they stand out as link and don’t just look like regular body text
  • A favicon would be nice!
  • Personal design thoughts:
    • On the /products page, I think it would be nice if you visually showed that the dropdowns are dropdowns. They kind of look like buttons now I think?
    • Font on the buttons for products doesn’t have the same font?
  • Please please please make links open in a new tab
  • Would be nice if the sorting and filtering was saved if I click a product and then go back
  • Would be nice to be able to edit my user profile (I saw the WIP 👍)

Needs fixing 👇

  • Accessibility: should score at least 95 on Lighthouse
  • I should not be able to create a new user if the user already exists
  • Potential bug:
    • I signed up with [email protected]
    • I signed up again with [email protected] (no error)
    • I deleted my profile (should maybe add a ”Are you sure?” popup hehe.)
    • I signed up again with [email protected] (successfully)
    • But… I can’t log in. I get ”The email or password is incorrect, please try again”
    • Is it because I didn’t fill in all required fields in the registration? Then you need to make it required in the frontend as well

I left some comments around your code as well, so have a look when you have time, but please fix the bold parts in the list above ☝️

Really great job and effort overall, you should be proud! 💪

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

This will be nice ✨

Choose a reason for hiding this comment

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

Very nice!

Comment on lines +25 to +55
if (!firstname) {
return res.status(400).json({ message: "Firstname is required." });
}

if (firstname.length < 2) {
return res
.status(400)
.json({ message: "Firstname must be at least 2 characters long." });
}

if (!lastname) {
return res.status(400).json({ message: "Lastname is required." });
}

if (lastname.length < 2) {
return res
.status(400)
.json({ message: "Lastname must be at least 2 characters long." });
}

if (!email) {
return res.status(400).json({ message: "Email is required." });
}

if (!address.street) {
return res.status(400).json({ message: "Streetname is required." });
}

if (!address.city) {
return res.status(400).json({ message: "City is required." });
}

Choose a reason for hiding this comment

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

You have a lot of validation here, which is really good, but what about if a user already exists with that specific email adress?

And would it be possible to make this more DRY somehow? 👀

Comment on lines +84 to +88
res.status(201).json({
message: `Your Registration was successfull ${user.firstname}. Please log in now.`,
id: user._id,
accessToken: user.accessToken,
});

Choose a reason for hiding this comment

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

Would be nice if a successful registration resulted in automatic login 😄

@@ -0,0 +1,35 @@
//THis ReviewCard is not implemented yet. It is here for future use.

Choose a reason for hiding this comment

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

Looking forward!

<IoIosArrowBack /> Continue shopping
</button>
</NavLink>
{orderHistory.length > 0 && <OrderHistory />}

Choose a reason for hiding this comment

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

Aha, so OrderHistory is just all the products in the cart right now? 👀

Comment on lines +38 to +62
const handleIncrement = (prevQuantity, id) => {
setNewQuantity((currentQuantity) => {
const newQty = prevQuantity + 1; // Increment the previous quantity by 1
updateCart(newQty, id); // Update the cart with the new quantity
return newQty; // Return the updated quantity
});
};

const handleDecrement = (prevQuantity, id) => {
setNewQuantity((currentQuantity) => {
const newQty = prevQuantity > 0 ? prevQuantity - 1 : 0; // Decrement the previous quantity by 1
if (newQty > 0) {
updateCart(newQty, id); // Update the cart with the new quantity
} else {
removeAllByIdFromCart(id); // Remove the item from the cart if quantity becomes 0
}
return newQty; // Return the updated quantity
});
};

const calculateTotalCost = (item) => {
const totalCost = item.quantity * item.product.price;
const roundedPrice = Math.ceil(totalCost * 100) / 100; // Round up to 2 decimal places
return `${roundedPrice}`;
};

Choose a reason for hiding this comment

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

These could be broken out into utils files

Comment on lines +41 to +69
const cardElement = elements.getElement(CardElement);
set({ isLoading: true });
try {
const response = await fetch(
"https://project-final-glim.onrender.com/stripe/create-payment-intent",
{
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({
amount: product.price * 100, // Convert price to cents
}),
}
);

if (!response.ok) {
throw new Error("Network response was not ok");
}

const data = await response.json();

const { clientSecret } = data;

const result = await stripe.confirmCardPayment(clientSecret, {
payment_method: {
card: cardElement,
},
});

Choose a reason for hiding this comment

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

Nicely implemented 💰

Choose a reason for hiding this comment

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

Well customized 👍

@linda-f
Copy link

linda-f commented Jun 26, 2024

Hi @HIPPIEKICK
We looked at the needs fixing points and fixed them.
Thank you for checking again.
We also tested accessibility on all pages and got at least 95 everywhere. See attached screenshots.
We also fixed the link for the favicon for the browser, you should be able to see it now ;)
desktop-about
desktop-cart
desktop-home
desktop-products
desktop-signup
mobile-about
mobile-cart
mobile-home
mobile-products
mobile-signup
desktop-profile
desktop-singlep
mobile-profile
mobile-singlep

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.

5 participants