adds simple todo list for review via PR #2
Conversation
joshdmiller
left a comment
joshdmiller
left a comment
There was a problem hiding this comment.
I like where you're going with this, but I put in a couple of things to chew on. Fundamentally, the question is this: how can you keep the "official" state in JavaScript, but only make the DOM reflect that state?
src/index.js
Outdated
| ], | ||
| }; | ||
|
|
||
| document.onload = init(); |
There was a problem hiding this comment.
This won't do what you think it will. Can you see why?
Also: document.onload will work, but it's not the same as the method we used in class (the DOMContentLoaded event). What do you suppose the difference is?
There was a problem hiding this comment.
I think I found the answer. It will work, but it's not the right thing to use because it would not fire until all elements have been loaded, including styles, scripts, and images:
The DOMContentLoaded event is fired when the initial HTML document has been completely loaded and parsed, without waiting for stylesheets, images, and subframes to finish loading. A very different event load should be used only to detect a fully-loaded page. It is an incredibly popular mistake to use load where DOMContentLoaded would be much more appropriate, so be cautious.
True?
There was a problem hiding this comment.
Indeed! I wouldn't say it's not the right thing to do, but we should be aware of the consequence. On this site, it makes no difference; on bigger apps, we spend a lot of time on performance perception and that could make a big difference.
src/index.js
Outdated
| let item = document.createElement('li'); | ||
| item.textContent = title; | ||
| if (complete === true) item.classList.add('todo--complete'); | ||
| item.addEventListener('click', event => event.target.classList.toggle('todo--complete') ); |
There was a problem hiding this comment.
It looks like we may be getting out of sync between what's in the DOM and what's in the state, right? If I mark one as complete, it will look complete, but in our state it will still be incomplete.
You could just toggle the flag in the callback there, but then you have to manually keep the two in sync. What might be a better way?
There was a problem hiding this comment.
AH, I see. THanks I'll take a look.
| const listNode = document.getElementById('list'); | ||
|
|
||
| const filterButton = document.getElementById('filter-button') | ||
| filterButton.addEventListener('click', (event) => { |
There was a problem hiding this comment.
If you needed to know if the app was filtered or not, how might you be able to tell with this implementation? For example, try to make the filter button say "Hide Completed" if they are not hidden, and "Show Completed" if they are.
There was a problem hiding this comment.
Not sure what you mean here. When you say have the button "say" something, are you referring to giving visual feedback that we are seeing a filtered list, or that we should track the state of "filtered/non-filtered" in state?
There was a problem hiding this comment.
I meant literally what was written on the button to provide feedback. Without it, the user won't really know if it's already filtered.
There was a problem hiding this comment.
OH, duh. You mean the actual text in the button. Got it. X)
|
Hey Josh, I made some major changes, pushed, and tried to issue another PR. Not sure if that's the way it's done, but this is where I ended up. There's a lot of new functionality that I believe solves the problems...maybe...? |
|
PS - if you have a resource for how to effectively make separate PR for situations like this, I'd appreciate it. For example:
|
2 participants
catching up on activities today. thanks for any tips on how to make this better!