Skip to content

Clarence#1

Open
chancyj wants to merge 3 commits intomasterfrom
clarence
Open

Clarence#1
chancyj wants to merge 3 commits intomasterfrom
clarence

Conversation

@chancyj
Copy link
Owner

@chancyj chancyj commented May 21, 2019

No description provided.

return (
/* When the user clicks submit, call the addTodo func */
<Container>
<form onSubmit={this.props.addTodo}>

Choose a reason for hiding this comment

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

You shouldn't really have to use a form component in React. The form's functionality is already accomplished by keeping track of a text value in state and then updating it whenever the user types in the box, as you do.

However, this currentText in the state of your TodoContainer is only used in this component, so it should live in this component rather than higher up. When you submit, then you can access the state from this component and add it to the todos in the state of the TodoContainer.

In general, always try to keep state as low as possible to where it's actually being used.

import React, { Component } from "react";
import { Input } from "./styles";

class SearchTodos extends Component {

Choose a reason for hiding this comment

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

Since this component has no state or methods, you can actually use a shorthand that we call an SFC (stateless functional component):

const SearchTodos = ({searchText, handleSearch}) => (
<Input
  value={searchText}
  placeholder="Search Todos"
  onChange={handleSearch}
/>
)

Choose a reason for hiding this comment

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

The arguments to this "function" are the props, passed as an object (I use destructuring in the argument to access the different props as individual variables), and the return value is JSX

class TodoItems extends Component {
// Function to create the list
makeList = todo => {
if (todo.status) {

Choose a reason for hiding this comment

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

Is status in todo to mark if the todo has been deleted or not? If so, you might consider just removing the todos from the list when the user clicks the button, since todos are irrecoverable in our app

Copy link
Owner Author

Choose a reason for hiding this comment

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

Status is to determine what to show when the user searches. True means show the item in our todo list

// Make sure the current item is not empty before adding
if (this.state.currentText !== "") {
const items = [
...this.state.items,

Choose a reason for hiding this comment

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

good use of the spread operator!

return t;
});

this.setState({

Choose a reason for hiding this comment

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

By setting the state of the items to only the matching todos of the query, we lose all of the other todos even after the user gets rid of their text. Instead of changing the state when processing the queries, I'd think about a way to filter which todos get displayed to the user more dynamically

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