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 To do #443

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

Project To do #443

wants to merge 25 commits into from

Conversation

AnnikaSonnek
Copy link

No description provided.

Copy link

@AntoniaGranit AntoniaGranit left a comment

Choose a reason for hiding this comment

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

Hi Annika! I loved your design this week, it also scales super well. I like the fact that you've implemented a deadline date-picker feature. Your code is smooth to read and the comments make it easy to understand what's happening where. I know you had trouble with the location of the TaskListStyles-styled component for some reason but I don't think it's a huge deal (even though I totally understand wanting everything in the right folder). The checkmark animation is particularly impressive, and I like that you managed to put the styling for it in its own component. Great job!

// this is an object that I call newTask that is sent into the items array (that contains all the tasks).

dispatch(tasks.actions.addTask(newTask));
// dispatching the addPokemon with the newPokemon as the argument

Choose a reason for hiding this comment

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

This comment is still from when we had a pokémon list! Haha

if (items.length === completedTasks.length) {
return ` ${completedTasks.length} / ${items.length}`
} else {
return `${completedTasks.length} / ${items.length} done`

Choose a reason for hiding this comment

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

I like this little detail that adds the word "done" if you haven't checked all your tasks yet.

// clearTask function calls the clearTasks-reducerfunction in the store.
return (
<HeaderWrapper>
<HeaderTitle>Stuff to do</HeaderTitle>

Choose a reason for hiding this comment

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

Love the font!

const taskList = useSelector((store) => store.tasks.items)
const dispatch = useDispatch()
const onTaskToggle = (taskId) => {
dispatch(tasks.actions.toggleTask({ taskId }));

Choose a reason for hiding this comment

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

I'm not a hundred percent sure but I don't think you have to do object destructuring here, I think "toggleTask(taskId)" would be enough.

return (
<TaskListWrapper>
<ul>
{taskList.map((singleTask) => {

Choose a reason for hiding this comment

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

One thing I noticed is that when you have a list of several tasks and you want to add a new one with a deadline, the calendar shows up underneath the list of tasks. Perhaps something that can be fixed with z-index?

@@ -0,0 +1,108 @@
/* eslint-disable jsx-a11y/label-has-associated-control */

Choose a reason for hiding this comment

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

The checkbox animation is fantastic!!

@@ -0,0 +1,20 @@
import styled from 'styled-components';

Choose a reason for hiding this comment

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

I looove this background! Very calm and a bit Twin Peaks

Choose a reason for hiding this comment

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

I didn't realize this is how you created the background image, super cool idea!!

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