Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File Browser II #57

Merged
merged 1 commit into from
Jun 17, 2020
Merged

File Browser II #57

merged 1 commit into from
Jun 17, 2020

Conversation

texodus
Copy link
Member

@texodus texodus commented Jun 15, 2020

No description provided.

Copy link
Contributor

@telamonian telamonian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it so far! A nice cleanup of the existing example that should be easier for someone new to follow.

Challenge: is there an equally nice way to implement the tree sort? Is there a simple sort implementation that keeps the pattern of modifying DATA in place?

In any case the sort stuff can be put off until the next PR, but I am curious

examples/file_browser.md Outdated Show resolved Hide resolved
examples/file_browser.md Outdated Show resolved Hide resolved
function styleListener() {
for (const td of window.regularTable.querySelectorAll("tbody th")) {
const meta = window.regularTable.getMeta(td);
td.classList.toggle("fb-directory", meta.value && DATA[meta.y].row[1] === "directory");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
td.classList.toggle("fb-directory", meta.value && DATA[meta.y].row[1] === "directory");
td.classList.add(meta.value && DATA[meta.y].row[1] === "directory" ? "fb-directory" : "fb-file");

The browser looks much more homey when there's both dir and file icons, so we should handle classes for both here. More verbosely, you could instead do

        td.classList.toggle("fb-directory", meta.value && DATA[meta.y].row[1] === "directory");
        td.classList.toggle("fb-file", meta.value && DATA[meta.y].row[1] === "file");

add is safe to use here, since the table cell classes get blanked on every redraw, right? Actually, that's a good question: does any table cell state carry over from invocation to invocation of the style listeners?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It hasn't been decided what regular-table's responsibility vis-a-vis resetting <td> state is yet. Ideally it would be none, but className and style are used currently by the internal auto-size and override code.


```javascript
function styleListener() {
for (const td of window.regularTable.querySelectorAll("tbody th")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the use of metadata in this loop, much cleaner/easier to follow than before

Comment on lines 113 to 268
tbody th:not(.fb-directory):not(:empty) {
padding-left: 20px;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tbody th:not(.fb-directory):not(:empty) {
padding-left: 20px;
}
tbody th:fb-file {
font-family: "Material Icons";
content: "text_snippet ";
}

The rest of the implementation for a file icon. Why did you put a white space at the end of the other icon ligatures?

@texodus texodus force-pushed the file-browser-rewrite branch 13 times, most recently from 1d589bf to 8281171 Compare June 17, 2020 06:24
@texodus texodus force-pushed the file-browser-rewrite branch from 8281171 to a38417c Compare June 17, 2020 22:52
@texodus texodus marked this pull request as ready for review June 17, 2020 23:12
@texodus texodus merged commit cf905e9 into master Jun 17, 2020
@texodus texodus deleted the file-browser-rewrite branch June 17, 2020 23:12
regularTable.addStyleListener(styleListener);
regularTable.addEventListener("mousedown", mousedownListener);
regularTable.addEventListener("scroll", () => {
regularTable.resetAutoSize();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, missing underscore. Should be

regularTable._resetAutoSize();

right? Super helpful that I spotted this right after you merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants