-
Notifications
You must be signed in to change notification settings - Fork 53
Ida and Tilde happyping #24
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
base: main
Are you sure you want to change the base?
Conversation
First layout
Added components LikeCounter, ThoughtItem and Spinner.
added margins to app-div
fetching and transforming data from API
Updated spinner
Updated spinner
Created MyLikeCounter
fixed the error message when clicking submit button
Changed back to happy instead of thankful and changed timing of mylikedmessage
Pulsating heart when liking post
added hover effect on posts
|
Sorry for the delay, we missed to do the PR! This is our Happy thoughts project @Idahel :) |
index.html
Outdated
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="./vite.svg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a custom Favicon can help add some individuality and can be a fun addition
| }; | ||
| setThoughts([newThought, ...thoughts]); | ||
|
|
||
| const audio = new Audio(plingSound); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this
| } | ||
| }; | ||
|
|
||
| const handleLike = async (id: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make the code easier to read, it could be a good idea to split it into a separate component. This way, it might also be easier to update things in the future.
|
Hey Ida and Tilde! I really liked your project. The page was visually easy to read and felt fun to explore the features. One thing I noticed is that since you changed the original design elements, it would have been nice to improve the accessibility of the page. For example, some of the color contrast could be higher to make things clearer, and turning the shadow into an animation took away from that a bit. I also saw that the layout didn’t use much semantic HTML, which makes it harder to follow the structure and also affects accessibility. It was a little tricky to figure out how the different parts of the page were grouped — like what was part of the main content, what was a section, what was a heading, and so on. In the code, I saw you used regular functions instead of arrow functions. That’s totally fine, but I was just curious if there was a reason for that. Maybe it’s easier with TypeScript? I don’t have a lot of experience with TypeScript myself, so it was actually cool to see how you used it. It gave me a better idea of what it looks like in a real project. Also, I liked that you used Tailwind. I haven’t used it much before, so it was fun to see it in action. The audio and animation features were great too — they made the page more interactive and fun. One suggestion would be to add some ARIA labels, especially for things like sounds or animations. That way, it’s easier for screen readers to pick up on what is happening when someone clicks or hovers. Another thing I noticed is that the app page felt a little hard to read. I think some of the functions could have been split into separate components to make things clearer. Right now, it’s a bit hard to tell which part belongs to what, and breaking things down might help with organization and readability. Overall though, I think it looks great. I really liked the direction you took with it. You used a couple of tools and styles I’m not super familiar with, and it gave me some ideas for what I might want to try out in future projects. |
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update your app with this new link (you can use cmd+shift+F to search for all the instances in a repo).
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just the a11y requirement left (95 in Lighthouse score and sufficient contrasts) 💪
|
Thank you! :) Did some accessibility improvements accordingly. |
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just made aware that a11y was not a requirement for this project, so I'm sorry for "forcing" you to improve it 😅
(https://happyping.netlify.app/)