Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Neat! I'm glad this worked out so quickly. Good work getting this going!
The main theme among my comments here is that I have identified some (hopefully easy) cleanup tasks to do in future PRs. When you merge this PR, can you please create issues to track those tasks? Then we can do those next.
|
|
||
| label.appendChild(checkbox); | ||
| label.appendChild(text); | ||
| filtersContainer.appendChild(label); |
There was a problem hiding this comment.
Overall, it looks like the strategy here is to populate the list of extension in the client, rather than when generating the HTML. Rather like the situation where we were generating SVG in the browser before, I think this could be a lot simpler if we simply generate the HTML for the checkboxes up front. This would avoid all the createElement and stuff, and it could be much easier to read.
I strongly suggest that we leave that change to a future PR as well! In the interest of getting this one merged, so the next step will be clear.
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
I found one unaddressed comment from my previous review, which I've marked within. And one more comment about some newly-changed code… it looks like this switched from using a JavaScript Map to a Set, but I'm not sure why we need either one.
| const items = Array.from(list.querySelectorAll('li')); | ||
| const activeExtensions = new Set(); | ||
|
|
||
| const extensionSet = new Set(); |
There was a problem hiding this comment.
I still don't really understand why we are using a Set here. It was previously a Map. Can we just construct an array and then sort it?
There was a problem hiding this comment.
No because this is essentially creating a list of extensions based on the list of instructions. Since a lot of the instructions have the same extension this automatically gets rid of the duplicate extensions. If I did an array I would have to take out the duplicates.
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Cool; seems like we're in business!
3 participants
Allows users to filter instructions based on extension