ITP Jan 25 | Katarzyna Kazimierczuk | Data Groups | Sprint 3 #505
Conversation
|
This branch seems to be based on an existing piece of work and it isn't clear exactly which code is up for review here. Do be careful to base branches from upstream main so that it's really clear what work is being submitted, and also make sure that commit messages explain the intent of individual changes (rather than things like "sprint3"). Was this meant to be a separate PR? |
|
@katarzynakaz Hello - it's still unclear here whether this is meant to be a combined PR. In the changes here there is work in the 'quote-generator', 'slideshow', and 'todo-list' directories - is this correct? |
@shieldo Hello, sorry, I didn't see your comment from yesterday. Todo: Slideshow: |
|
On separate note, it is a good practice, when submitting a PR, check the self checklist and give a brief description to the PR.
|
There was a problem hiding this comment.
There's some really good work here - writing code that updates aspects of the DOM based on user actions is quite a hard thing to manage, and you've taken the sensible approach of rendering either the image you are showing (in the slideshow) or the state of the todos (for the todo list) in a separate function, which is a scalable way of going about things that follows a similar approach to how web rendering libraries such as React work.
There's a couple of places here that don't quite work, but you've done well here.
| <button type="button" id="backward-btn">Backwards</button> | ||
| <button type="button" id="forward-btn">Forward</button> | ||
| </div> | ||
| <button type="button" class="small" id="auto-forward-bt"> |
There was a problem hiding this comment.
It looks like there is a mistype here that is causing the JavaScript to fail. Can you spot it? Looking in the Console tab of your browser's dev tools might help!
|
|
||
| //automated slideshow button event listeners | ||
| autoForwardBtn.addEventListener("click", () => setInterval(forward, 1500)); |
There was a problem hiding this comment.
This would probably work pretty well, but our forward function here just changes the index of the picture that would be shown if updatePicture were called. The idea of separately updating the picture is a pretty good one, and sometimes that pattern could be what you want, but maybe in this case it might be better to call updatePicture at the end of the forward and back functions? It seems that every time we call forward, for example, we're going to want to change the display of the image. If things were to get more complicated then maybe you'd want to call the updatePicture function separately.
| checkbox.checked = todoObject.completed; | ||
|
|
||
| // clicked, toggle competed or not completed | ||
| checkbox.addEventListener("change", () => { |
There was a problem hiding this comment.
Using a checkbox like this works pretty well here - and checking the box does have the effect of marking the todo as done!
The instructions do say to use
<i class="fa fa-check" aria-hidden="true"></i>
<i class="fa fa-trash" aria-hidden="true"></i>here to show a checkbox and a trash symbol for deleting a todo.
| document.getElementById("add").addEventListener("click", addNewTodo); | ||
|
|
||
| //ignore this part | ||
| // nope :) :) |
shieldo
added
Reviewed
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.