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

Happy Thoughts By Anna #95

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

Conversation

Anna2024WebDev
Copy link

…css files, fetched the thoughts data from API
…/max number of characters, also added styling to some components
…rm and setup the elements in correct order and changed color contrast
Copy link
Contributor

@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.

Good job with your happy thoughts app Anna! You’ve successfully copied the design as well as fetched from an API in React 🥳

Really good error handling as well as structure of your components. Just remember that your folders should be in lowercase (unless you structure your files so that you have one folder called e.g. Header with a Header.jsx and a Header.css in it). A next step for better structure would be to break it down even further, e.g. abstracting the API calls and helper functions such as your timeAgo function (kudos on coding that from scratch by the way!)

I can see that you put some thought into accessibility as well - great!

Overall, your code is easy to follow but I would like to add some notes on clean code:
Be consistent with single or double quotes and whether or not you’re using semicolons
Remove unused code
Order your imports in the following order:

// React and core libraries
import { useState, useEffect } from 'react';

// Components
import { Header } from"./Components/Header";
import { ThoughtForm } from "./Components/ThoughtForm";
import { ThoughtList } from"./Components/ThoughtList";

// Assets (CSS, images)
import "./Styles/App.css";

Changes requested

  • Clean up code

max-width: 95%;
padding: 10px;
box-sizing: border-box;
resize: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐️

@Anna2024WebDev
Copy link
Author

Thank you for your great feedback and comments @HIPPIEKICK!

I have now;

  1. Removed semicolons since I prefer not to use it if not necessary (such in CSS)
  2. Use double quotations everywhere I can.
  3. Removed unused code such as API requests for likes to two components. I had added it both in the ThoughtList component as well as the ThoughtHeart component. It was removed from the ThoughtList since I have the like changes in the HeartComponent.
  4. Rearranged the components in correct order and changed the names of the folders to lowercase.

Can you please have a look again?

ps. The timeAgo function I got help from chatGPT with, maybe I was not clear enough in the readme file about that. 🙈

Copy link

@JoyceKuode JoyceKuode left a comment

Choose a reason for hiding this comment

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

I was really impressed overall with your happy thoughts app, Anna! 🩷 You did a great job hitting all the requirements and matching the design but with your own flair. Your code looks really organized and you have such clarity with your comments so I appreciate being able to follow along easily. I just had a couple of suggestions to deal with separation of concerns and maintainability. Hope this helps as our projects get larger and more complex.

Keep up the fantastic work, gold star for you ! 🌟 I'm excited to see what you'll create next 😊

src/App.jsx Outdated
<div className="App">

{isLoading && <h1>Loading...</h1>} {/* Show loading message while data is being fetched */}
{/* Render the app content once loading is complete */}

Choose a reason for hiding this comment

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

Nice job with showing the loading message ⭐

Choose a reason for hiding this comment

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

What a cute icon ! 😊🩷

<h2>What is making you happy right now?</h2>
<form onSubmit={handleSubmit}>
{/* Label for accessibility (hidden visually but helps screen readers) */}
<label htmlFor="thoughtText" className="visually-hidden">Share your happy thought:</label>

Choose a reason for hiding this comment

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

Nice with the extra attention to a11y 👏

<span
className={`heart-count ${liked ? "liked" : ''}`}
onClick={handleLike}
style={{ cursor: "pointer" }}>

Choose a reason for hiding this comment

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

I think it's generally considered best practice to put styling in CSS for separation of concerns, reusability and maintainability. You have declared a className here which you can use to add style in the ThoughtHeart.css file

border-radius: 50%;
padding: 10px;
margin-right: 5px;
}

Choose a reason for hiding this comment

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

I think the heart-count style I mentioned in the ThoughtHeart.jsx file can be used here, and you can replace span with .heart-count. Using a specific class that you already created rather than the generic span selector can improve maintainability and clarity of your code. Something like this maybe ?

.heart-count {
  background-color: #EAEAEA;
  border-radius: 50%;
  padding: 10px;
  margin-right: 5px;
/* Add cursor: pointer here */
  cursor: pointer; 
}

.heart-count.liked {
  /* Add styles for the liked state */
}

// ThoughtList Component: Responsible for displaying a list of thoughts passed down from the App component.

// Each component has it's separate CSS file*/ }

Choose a reason for hiding this comment

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

Thank you for this very clear explanation of your components. 🙏

max-width: 95%;
/* Use a smaller max-width on mobile */
padding: 10px;
}

Choose a reason for hiding this comment

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

You have styles here for .thought-form and .thought-list, but you also have ThoughtForm.css and ThoughtList.css files... You can also create a media query in the other CSS files. Consolidating all styles for the .thought-form class into the ThoughtForm.css file and the .thought-list class into the ThoughtList.css file will improve consistency and avoid conflicts that can lead to unexpected behavior.


.visually-hidden {
display: none;
}

Choose a reason for hiding this comment

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

An example of the comment I made in your App.css file :

You can move the media query from the App.css over here like this (and similarly create a media query in your ThoughtList.css for the .thought-list class), and that way changes you might make to a class will be applied consistently across your project (plus for larger projects, it makes it easier to find in a single file 🔦):

@media (max-width: 600px) {
  .thought-form {
    max-width: 95%;
    padding: 10px;
  }
}

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.

3 participants