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

To do list - Maja Z #439

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

To do list - Maja Z #439

wants to merge 15 commits into from

Conversation

majazimnoch
Copy link

Copy link

@smirrebinx smirrebinx left a comment

Choose a reason for hiding this comment

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

Overall this is a nice looking app. Good job! No console errors, great! It's good practice to have an h1 at the top of the page to display the main heading of a web page so you might consider adding that :-)

addToDo: (store, action) => {
// store.items.push(action.payload)
store.items = [action.payload, ...store.items];
localStorage.setItem('toDoList', JSON.stringify(store.items));

Choose a reason for hiding this comment

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

I like that you added the localStorage.

Find me in src/app.js!
</div>
<Provider store={store}>
<Wrapper>

Choose a reason for hiding this comment

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

The App.js looks nice and clean with the mounted components.

</TopDiv>
<StyledForm onSubmit={onFormSubmit}>
<label htmlFor="addToDoInput">
Time to add a new task to the list and get things done <br />

Choose a reason for hiding this comment

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

I'm not really sure why you've added the
here but instead of
you could use a

for every new paragraph (best practice) :-))

justify-content: center;
`

export default function DateToday() {

Choose a reason for hiding this comment

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

Nice touch with the current date.

@@ -0,0 +1,77 @@
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.

Good use of a global component.

<li key={singleToDo.id}>
<ToDoListBox>
<LeftToDo>
<input id={`toDo_with_id${singleToDo.id}`} type="checkbox" value={singleToDo.IsDone} onChange={() => onIsDoneCheckboxToggle(singleToDo.id)} />

Choose a reason for hiding this comment

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

To increase user accessibility and make the page more user-friendly you can connect the label with the checkbox so that the user can click on the text for the checkbox and not just on the actual checkbox. I think you need to wrap the text (that you have in a span maybe?) inside of the label. You might also want to add a focus to the checkbox in CSS so that keyboard users see where they are when tabbing.

position: absolute;
width: 25px;
height: 25px;
outline: 2px solid var(--buttonGreen);

Choose a reason for hiding this comment

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

Wave complains about color contrast errors for the checkboxes, consider using a darker color for the checkbox borders.

import { useSelector } from 'react-redux'
import { BottomToDo, HCounter } from './GlobalStyles';

const Counter = () => {

Choose a reason for hiding this comment

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

To get the counter working you need to start the counter at 0. You can try something like this:
`const Counter = () => {
const todos = useSelector((store) => store.todos.items);
const completedTodos = todos.filter((todo) => todo.isDone);

return (


Completed todos: {completedTodos.length}



);
};
`

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