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

"Modified" file browser column should be "Last Modified" when there is enough space to display #16197

Closed
jtpio opened this issue Apr 19, 2024 · 7 comments · Fixed by #16207
Closed
Assignees
Labels
bug tag:Release Blocker A must-have bug for the milestone to which it is tagged
Milestone

Comments

@jtpio
Copy link
Member

jtpio commented Apr 19, 2024

Description

This affects Notebook 7 downstream: jupyter/notebook#7333 (comment)

In Notebook 7 there is plenty of space to display the full "Last Modified". Also it would be better to keep "Last Modified" in Notebook 7 for parity with the classic notebook, at least for now.

Reproduce

Available in 4.2.0b2.

Currently defined here in the code base:

const modified = this.createHeaderItemNode(trans.__('Modified'));

Expected behavior

The column should say "Last Modified" when the file browser is displayed on the whole page.

There could be the same logic as the column content to reduce it to just "Modified" when the file browser width is smaller.

Context

Reported in:

@jtpio jtpio added bug status:Needs Triage Applied to new issues that need triage labels Apr 19, 2024
@jtpio
Copy link
Member Author

jtpio commented Apr 19, 2024

There could be the same logic as the column content to reduce it to just "Modified" when the file browser width is smaller.

@JasonWeill @krassowski I didn't have time to review #15948 before it got merged, but was expecting the logic would make the header shorter only when necessary, like for the column content.

Adding as release blocker so we have a chance to look into this.

@jtpio jtpio added the tag:Release Blocker A must-have bug for the milestone to which it is tagged label Apr 19, 2024
@jtpio jtpio added this to the 4.2.0 milestone Apr 19, 2024
@krassowski
Copy link
Member

What would be the right threshold? I think it should be different from the thresholds used for the column content, maybe something on the order of 400-500px?

@jtpio
Copy link
Member Author

jtpio commented Apr 19, 2024

maybe something on the order of 400-500px?

Sounds good. The use case is mostly for displaying the file browser on the full page on desktop, which is usually more than these numbers. And it would be fine if it gets shorten to "Modified" on mobile for example.

@krassowski
Copy link
Member

@JasonWeill do you have time to pick it up?

@JasonWeill
Copy link
Contributor

I can take a look at this.

@JasonWeill JasonWeill self-assigned this Apr 19, 2024
@JasonWeill JasonWeill removed the status:Needs Triage Applied to new issues that need triage label Apr 19, 2024
@JasonWeill
Copy link
Contributor

@jtpio @krassowski I've been looking into this, connecting the onResize handler to a function that measures the header element's size. This correctly assigns "Modified" and "Last Modified" on resize, but on initial load, only the header that's specified in populateHeaderNode gets used. Inside of populateHeaderNode, there isn't enough information available to discern how wide the header node would be before it is added to the DOM.

In an earlier version of #15948, I was using container queries to display one of three different timestamps for each file. This was a burden for performance, since every file listing had to have three modified timestamps. Because there's only one column header for the modified column, would it be OK to render the column header twice, and then use CSS rules to render only one of them if the header is >= or < 300 px?

@JasonWeill
Copy link
Contributor

PR #16207 uses the container queries approach, not a JS/TS approach, to fix this bug. I included a wide layout in JupyterLab in the pull request, but I'm not sure whether this would be suitable for Notebook. I'd appreciate if you could take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tag:Release Blocker A must-have bug for the milestone to which it is tagged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants