Conversation
| @@ -0,0 +1,74 @@ | |||
| // Updating the first task in the this.state.tasks array | |||
There was a problem hiding this comment.
Can explain the purpose of this document?
| <noscript>You need to enable JavaScript to run this app.</noscript> | ||
| <div id="root"></div> | ||
| <!-- | ||
| <div class="background-container"> |
There was a problem hiding this comment.
is this a bootstrap class? If not, what purpose does it serve? I don't see any styling anywhere for it.
There was a problem hiding this comment.
Used it but deleted the css
| .app-container { | ||
| display: flex; | ||
| flex-direction: column; | ||
| height: 100vh; /* height of 100vh ensures that the layout spans the full height */ |
| @@ -1,19 +1,105 @@ | |||
| .App { | |||
| text-align: center; | |||
| /* App.css */ | |||
| .task-completed { | ||
| flex: 1; /* Equal distribution of space */ | ||
| box-sizing: border-box; /* Include padding and border in the width calculation */ | ||
| width: calc(33.33% - 40px); /* 33.33% width with 20px margin on each side */ |
There was a problem hiding this comment.
this seems responsive but might not actually be. 40px on a mobile screen might be a lot.
| cols="20" | ||
| value={this.state.task} | ||
| onChange={this.handleChange} | ||
| style={{ height: "400px" }} |
There was a problem hiding this comment.
Fixed height might be tricky. What if the user input is more than that? Unless every row has a max of 100px? Overall, is this necessary? Maybe minHeight would be a bit better here. Also, why use inline-styling instead of the css file?
| <button onClick={resetLocalStorage}>Reset to Default</button> | ||
| </div> | ||
| <TaskComposer addTask={addTaskToDo} taskLength={tasks.length} /> | ||
| {tasks && tasks.length > 0 ? ( |
There was a problem hiding this comment.
Since tasks is an array, the first condition is always true. [] is a truthy value in JavaScript
There was a problem hiding this comment.
| {tasks && tasks.length > 0 ? ( | |
| {tasks.length ? ( |
this would be sufficient here
| {tasks.map((task) => ( | ||
| <Task | ||
| key={task.id} | ||
| {...task} |
There was a problem hiding this comment.
Why add all task properties here as props? I would rather pass the whole task as prop, and then use it in the Task component instead of accessing all properties via component props individually.
| showButton={false} | ||
| showButtonInProgress={true} |
There was a problem hiding this comment.
If you would include these booleans in a task's data structure as properties, you wouldn't need 4 different tasks list components, but would be able to run this map here completely dynamically and only use a single prop for displaying a certain button or not. Hardcoding like so will lead to problems with extending upon your software in the future.
| this.props.updateTask(this.props.id, updatedTask); | ||
|
|
||
| // Close the update form by calling the callback function by setting isEditing in Task.js to false | ||
| this.props.onUpdateComplete(); |
There was a problem hiding this comment.
is onUpdateComplete a callback for updateTask here? I do not think so. The both functions could potentially execute simultaneously, depending on what the functions do. It is not guaranteed that a task is updated before the complete function runs.
2 participants
No description provided.