Enable multiple levels #10
Conversation
src/main.js
Outdated
| @@ -2,7 +2,19 @@ function convertItemsToUnorderedList(listItems) { | |||
| const ulElement = document.createElement("ul"); | |||
| listItems.forEach(item => { | |||
| const liElement = document.createElement("li"); | |||
| liElement.textContent = item.name; | |||
| liElement.classList.add("fs-api-entry"); | |||
| if (item.type === "file") { | |||
There was a problem hiding this comment.
Assuming we are are getting valid input every time, we can ditch the if statement here in favor of a template literal (learn more):
liElement.classList.add(`fs-api-${item.type}`);
src/main.js
Outdated
| } else { | ||
| liElement.classList.add("fs-api-directory"); | ||
| } | ||
| const spanElement = document.createElement("span"); |
There was a problem hiding this comment.
Can you please rename spanElement to something more semantic (e.g. nameElement)?
We favor variable names describing the content of each variable, instead of their technical aspects.
src/main.spec.js
Outdated
| expect(ul.children[3].children[0].textContent).toBe("SFile.sth"); | ||
| expect(ul.children[4].children[0].textContent).toBe("test.go"); | ||
| expect(ul.children[5].children[0].textContent).toBe("Troll.go"); | ||
| for (i = 0; i < ul.length; i++) { |
There was a problem hiding this comment.
First, we should not use ul.length, as it is not in the spec of the HTMLUListElement (read the spec).
Next, we should simplify this. An example that could work:
for (const li of ul.children) {
// ...do thing here
}
src/main.spec.js
Outdated
| expect(ul.children[4].classList.contains("fs-api-file")).toBe(true); | ||
| expect(ul.children[5].classList.contains("fs-api-file")).toBe(true); | ||
|
|
||
| for (i = 0; i < 6; i++) { |
There was a problem hiding this comment.
We should simplify this as well. Example:
for (const li of ul.children) {
// ...do thing here
}
src/main.spec.js
Outdated
| trees[0].children[1].children[1].children[1].children[0].textContent | ||
| ).toBe("yarn.lock"); | ||
|
|
||
| //expect(trees[0].children[1].children[0].children[1].textContent).toBe("yarn.lock"); |
src/main.spec.js
Outdated
| expect(trees[0].children[5].children[0].textContent).toBe("Troll.go"); | ||
|
|
||
| expect( | ||
| trees[0].children[1].children[1].children[0].children[0].textContent |
There was a problem hiding this comment.
Hmmm... I think this is a bit illegible here. What about replacing trees[0].children[1].children[1] with something more semantic and reusing it also in the test below?
e.g.
const vendorChildren = trees[0].children[1].children[1]; // or vendorSubTree, or something else that sounds more relevant
src/main.spec.js
Outdated
| expect(trees[0].children[4].classList.contains("fs-api-file")).toBe(true); | ||
| expect(trees[0].children[5].classList.contains("fs-api-file")).toBe(true); | ||
|
|
||
| for (const li of trees) { |
There was a problem hiding this comment.
I guess here you mean const tree of...
src/main.js
Outdated
| @@ -2,7 +2,15 @@ function convertItemsToUnorderedList(listItems) { | |||
| const ulElement = document.createElement("ul"); | |||
| listItems.forEach(item => { | |||
| const liElement = document.createElement("li"); | |||
| liElement.textContent = item.name; | |||
| liElement.classList.add("fs-api-entry"); | |||
There was a problem hiding this comment.
You can squash these two .add calls into one 😄: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Methods
src/main.spec.js
Outdated
| expect(trees[0].children[3].children[0].textContent).toBe("SFile.sth"); | ||
| expect(trees[0].children[4].children[0].textContent).toBe("test.go"); | ||
| expect(trees[0].children[5].children[0].textContent).toBe("Troll.go"); | ||
| const vendorChildren = trees[0].children[1].children[1]; |
There was a problem hiding this comment.
Is this trees[1]? If yes, we should use this instead.
src/main.spec.js
Outdated
| expect(ul.children[3].textContent).toBe("SFile.sth"); | ||
| expect(ul.children[4].textContent).toBe("test.go"); | ||
| expect(ul.children[5].textContent).toBe("Troll.go"); | ||
| expect(trees[0].children[0].children[0].textContent).toBe("Dir"); |
There was a problem hiding this comment.
Can we rename trees[0] to something more semantic (e.g. rootTree, mainTree)
No description provided.