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

Project Happy Thoughts - Hannah Ek #435

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Nahnahke
Copy link

No description provided.

Copy link

@mvfrid mvfrid left a comment

Choose a reason for hiding this comment

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

Looking goooood! Nice clean design. The only thing I noticed, is that the page is not responsive for the smaller mobile sizes. From what I can see, your max width is 400px. The rest is looking and working great!

I checked the accessibility on your site and it worked great overall. I added some comments on some areas where you easily could improve the screen reader experience to get "full pott".

Your code is very clean and concise. Both the JS and CSS parts. It's easy to follow, both in the code itself and in the inspector tool. Very nicely done!

One improvement suggestion if you want to lift your work even more, is to add another class that is activated when the user has liked a post, maybe a slightly darker color or similar? To indicate which posts the user has already liked.

Good job Hannah!

Comment on lines +7 to +9
<meta property="og:title" content="Project Happy Thoughts - Hannah Ek"> <!-- OG (open graph) tags are snippets of code that provide meta data about a web page, mainly to social media platforms (ex facebook, linkedin, instagram)-->
<meta property="og:description" content="Project Happy Thoughts, a project by Hannah Ek, a student @Technigo Web Development Bootcamp spring 2023">
<meta property="og:image" content="https://i.postimg.cc/kMzq5ttr/Screenshot-2023-03-26-at-09-19-59.png">
Copy link

Choose a reason for hiding this comment

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

Nice job including the og tags! I suggest you check what it looks like through this page: https://www.opengraph.xyz/url/https%3A%2F%2Fproject-happy-thoughts-hannah-ek.netlify.app%2F

I think it would look even better if you make the image wider with white space, then more of it will be shown and not "cut".

Comment on lines +8 to +12
if (newThought.length < 5) {
return alert('Get on that keyboard, more than 5 characters, please!')
} else if (newThought.length > 140) {
return alert('Are you some sort of hacker?! No more than 140 characters, please!')
} else {
Copy link

Choose a reason for hiding this comment

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

Love the messages haha! Also great and condense way of writing the error conditionals.

.then((response) => response.json())
.then(() => {
setNewThought('');
setTimeout(() => window.location.reload(), 3000)
Copy link

Choose a reason for hiding this comment

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

Really great idea to use the timed function here!

};

return (
<section className="main-container">
Copy link

Choose a reason for hiding this comment

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

I like that you re-used the className for both the NewThought section and the ThoughtFeed section. Great way of keeping down the amount of code! It shows that you have really thought it through :)

font-size: 15px;
}

textarea {
Copy link

Choose a reason for hiding this comment

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

To prevent users from resizing the textarea by dragging the corner, you can use the resize property and set its value to none. This will disable the resizing feature of the textarea.

resize: none;

import Spinning from './Spinning';
// import { Loading } from './Loading'

const API = 'https://happy-thoughts-ux7hkzgmwa-uc.a.run.app/thoughts'
Copy link

Choose a reason for hiding this comment

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

Nice idea to have the fetch adress outside the function, makes it look a lot cleaner inside the fetch!

<button type="button" className={thought.hearts === 0 ? 'noLikesBtn' : 'likesBtn'} onClick={() => HandleLike(thought._id)}>
<span role="img" aria-label="Like this post">💗</span>
</button>
<span className="sum-hearts">x {thought.hearts}</span>
Copy link

Choose a reason for hiding this comment

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

I listened to your app and this sounds a bit funny, it really says e.g. "x four". Could you maybe add an aria-label to it, so that the speaker reads "likes times four" or similar?

<span className="sum-hearts" aria-label={likes times ${thought.hearts}}> x {thought.hearts} </span>

value={newThought}
onChange={(event) => setNewThought(event.target.value)} />
</label>
<p className={newThought.length > 140 ? 'counter' : 'counter'}>{newThought.length} / 140</p>
Copy link

Choose a reason for hiding this comment

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

Also a comment regarding the speak-reader. Now it never reads the "/", which makes it confusing to understand the length that is written and allowed. I recommend adding an aria-label to this section.

<p className={newThought.length > 140 ? 'counter' : 'counter'} aria-label={newThought.length} out of 140>{newThought.length} / 140</p>

Comment on lines +7 to +14
<>
<section>
<NewThought />
</section>
<section>
<ThoughtFeed />
</section>
</>
Copy link

Choose a reason for hiding this comment

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

If you look in the inspector, the

seem to not be needed here. I think you could rewrite it like this:

<> <NewThought /> <ThoughtFeed /> </>

Comment on lines +92 to +100
.heartEmo {
font-size: 12px;
transition: font-size 0.1s ease-in-out;
}

.heartEmo:hover {
cursor: pointer;
font-size: 18px;
}
Copy link

Choose a reason for hiding this comment

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

I can't find "heartEmo" anywhere other than in the css file. Is it supposed to be named something else?

Also, " cursor:pointer; " can be added directly to the non-hover property, since it will be the same outcome :)

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