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

Web Dev Bootcamp spring 2024, week 12 - Project Collab - Vittoria & Sofia #44

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

Conversation

sofia32057
Copy link

Netlify link

Thrive Fitness on Netlify

Collaborators

[vittoriamatteoli]

Copy link

@El1an3 El1an3 left a comment

Choose a reason for hiding this comment

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

Good job Sofia & Vittoria.
The website works on all devices and your code is very clean. I'm always very impressed with your work. I have learnt a few things from reviewing your code. Thank you.

I would have liked to have two different folders for the components to distinguish between the actual sections and the reusable parts. But that's just a personal preference.

<>
<StyledBurger
onClick={toggleMenu}
aria-label="Toggle menu"
Copy link

Choose a reason for hiding this comment

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

Thx about the reminder. I always forget to use aria-label.

@@ -0,0 +1,160 @@
import styled, { keyframes } from "styled-components";
Copy link

Choose a reason for hiding this comment

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

nice! I didn't use keyframes. have to look into it for animations.

return (
<StyledSection>
<H2>Our services</H2>
<CarouselContainer>
Copy link

Choose a reason for hiding this comment

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

Nice solution for your carousel.

<CarouselContainer>
<StyledCarousel>
{cardContent.map(card => (
<React.Fragment key={card.heading}>
Copy link

Choose a reason for hiding this comment

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

So much I have to look up... ;-)
Never used React.Fragment myself -> seems to be handy to avoid a lot of wrapping elements.

<Nav>
<ul>
<li>
<Link to="/contact">Contact</Link>
Copy link

Choose a reason for hiding this comment

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

good work, even implementing the routes.

import { handleResize } from "../utils/handleResize";

export const Home = () => {
const [windowWidth, setWindowWidth] = useState(window.innerWidth);
Copy link

Choose a reason for hiding this comment

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

I like how you handle the resize here. I used it directly in the components. Your approach makes the code a lot cleaner.

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Hello Vittoria & Sofia, congratulations for completing your week 12 project. It looks ✨ awesome ✨ The site looks very close to the design and followed most of our requirements. How did you like Styled Components? Seems like you tackled it in the right way and implemented a great result! Now the design is not pixel perfect, but that's okay, here's some aspects that need to be taken into consideration to meet the requirements:

  • Images resolution: use always format.svg for images not.jpg, this will ensure HQ on any screen size
  • accessibility scored 80 - should be at least 95
  • make sure icon and button sizes, margins and spaces are proportioned. Right now the elements in the nav bar and footer appear quite big and the section looks crowded compared to the original design. It would be enough to reduce the font-size and icons width.

Please fix these few things and you're good to 🚀 🥇

@sofia32057
Copy link
Author

Hi @AntonellaMorittu!
For which breakpoint does the font sizes not match?

@sofia32057
Copy link
Author

Hi again @AntonellaMorittu
SVG for photos doesn't sound right and I can't find anything online supporting this would be a good practice. Can you elaborate?

@AntonellaMorittu
Copy link

Hi 👋 for example difference between design and implementation:

Screenshot 2024-05-07 at 13 47 06 Screenshot 2024-05-07 at 13 47 42

Also it's good to know not to rely too much on css rules suggested on Figma, many are auto-generated and not be accurate at times, visually you see when the proportions are off.

Regarding the svgs, if available in the design (like in this example), use SVG format. SVGs are vector-based — built from a complex mathematical network of lines, dots, shapes, and algorithms. They can expand to any size without losing their resolution therefore you don't lose image resolution.

Screenshot 2024-05-07 at 13 55 05

These are not critiques Sofia, we're in a learning environment and I am just trying to guide you to achieve the best result possible. Good luck :)

@sofia32057
Copy link
Author

@AntonellaMorittu Thanks for the clarification!

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