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

Much smaller "Last Modified" column, date #15948

Merged
merged 41 commits into from
Apr 18, 2024

Conversation

JasonWeill
Copy link
Contributor

@JasonWeill JasonWeill commented Mar 8, 2024

References

Fixes #15947.

Code changes

Shortens dates to extremely short units, as short as one character (days becomes d) and no longer than three characters (min, mon). All numbers are assumed to be negative; if a file's timestamp is in the future, a + is prepended.

User-facing changes

Before the change, the relative timestamp was so long that the "last modified" minimum width was wider than the file name container's:

long-date

Now, the last modified column is shortened to "Modified", and is narrower:

Screenshot 2024-03-12 at 5 36 10 PM

One of three dates is displayed based on how wide the "Modified" column is:

3-step

Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@andrii-i
Copy link
Contributor

andrii-i commented Mar 8, 2024

In my subjective opinion, middleground between this PR and version proposed in #15947 would be a good compromise between saving space and maintaining readability: "Modified" for the column name and "7 months" or at least "7 mo." for value format (with a space and a dot).

@krassowski
Copy link
Member

I like the idea of having a shorter form when needed. I do not like forcing the shorter form and "Mod" header by default. Internationalisation-wise it would be great if we could off load this to self-contained package which supports such short format with translations.

@jtpio
Copy link
Member

jtpio commented Mar 8, 2024

having a shorter form when needed.

Or at least configurable. For example Notebook 7 would likely keep the longer version by default.

@krassowski
Copy link
Member

Of note, this filebrowser is also used in Jupyter Notebook where it takes full-width of the page. It can be also used by user widgets and in other places where the short form does not make sense.

On the other hand, the file selection dialog like FileDialog.getOpenFiles should probably always use the short form.

@krassowski krassowski added this to the 4.2.0 milestone Mar 8, 2024
@andrii-i
Copy link
Contributor

Here’s how macOS does it, timestamp format adjusted based on the width of the column.

image
image
image

@JasonWeill
Copy link
Contributor Author

The same i18n-friendly library we use also supports a super-compact format called narrow, although it still includes ago or in:

const rtf1 = new Intl.RelativeTimeFormat('en', { style: 'narrow' });
console.log(rtf1.format(3, 'quarter'));
console.log(rtf1.format(-1, 'day'));

causes log messages:

> "in 3q"
> "1d ago"

@JasonWeill
Copy link
Contributor Author

With the most recent change, the name and modified columns flex in width, and based on the container width, one of three dates is displayed.

Notably, the csstree/stylelint-validator module we use flags container queries as invalid CSS, even though all modern browsers support them, and even though we're using the newest version of this module.

3-step

@krassowski
Copy link
Member

krassowski commented Mar 14, 2024

This UX is nice and who doesn't love the container queries (it's awesome that we finally can use them!) but rendering the three variants will have three times the cost. The file browser already has performance issues when the list of files is long - I would strongly suggest to attempt to reimplement the conditional rendering on JS rather than CSS level.

Notably, the csstree/stylelint-validator module we use flags container queries as invalid CSS, even though all modern browsers support them, and even though we're using the newest version of this module

If you do stick to CSS queries you can add ignore comments disabling the validator around the problematic rules.

@krassowski
Copy link
Member

On the other hand, maybe it is fine to merge this as-is and look into performance improvements as a follow up.

One thing that would be needed here is visual snapshots at the two new levels.

@JasonWeill
Copy link
Contributor Author

please update galata snapshots

Copy link
Contributor

Galata snapshots updated.

@JasonWeill
Copy link
Contributor Author

please update documentation snapshots

Copy link
Contributor

Documentation snapshots updated.

@JasonWeill JasonWeill linked an issue Mar 20, 2024 that may be closed by this pull request
@JasonWeill
Copy link
Contributor Author

please update snapshots

Copy link
Contributor

Galata snapshots updated.

@JasonWeill JasonWeill marked this pull request as ready for review March 21, 2024 01:57
@krassowski
Copy link
Member

I think the commit "Adds test case for string replacement that contains query string" was meant to go to #15881 branch

@JasonWeill
Copy link
Contributor Author

Thanks for catching this! Reverted here and cherry-picked in the #15881 branch.

@krassowski
Copy link
Member

bot please update snapshots

Copy link
Contributor

Documentation snapshots updated.

Copy link
Contributor

Galata snapshots updated.

Copy link
Member

Choose a reason for hiding this comment

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

Odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this change is intended, but the new screenshot certainly looks better 😄

@krassowski krassowski closed this Apr 18, 2024
@krassowski krassowski reopened this Apr 18, 2024
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @JasonWeill. Good to merge once all green.

@JasonWeill JasonWeill merged commit f23e308 into jupyterlab:main Apr 18, 2024
80 of 81 checks passed
@krassowski
Copy link
Member

The only failure:

[jupyterlab] › test/jupyterlab/sidebars.test.ts:42:9 › Sidebars › Open Sidebar tab jp-running-sessions 

should be fixed by merge of #16188 (review welcome!)

@@ -2297,7 +2368,7 @@ export namespace DirListing {
const trans = translator.load('jupyterlab');
const name = this.createHeaderItemNode(trans.__('Name'));
const narrow = document.createElement('div');
const modified = this.createHeaderItemNode(trans.__('Last Modified'));
const modified = this.createHeaderItemNode(trans.__('Modified'));
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to keep the original Last Modified here?

Since the file browser is also used in Notebook, and there is plenty of space to display it.

Copy link
Member

Choose a reason for hiding this comment

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

gderocher pushed a commit to gderocher/jupyterlab that referenced this pull request Apr 26, 2024
* Revert "Alternate description for disabled filters"

This reverts commit 059fb7e.

* Revert "Revert "Alternate description for disabled filters""

This reverts commit 27ed1d0.

* Uses extremely short units, shrinks the Last Modified column by 62 px

* Lint fixes

* Uses standard library in 'narrow' mode

* Three-step container query to display longer or shorter date

* Update Playwright Snapshots

* Update Playwright Snapshots

* WIP: Creates test files

* Adds small and large sidebar tests specifically for filebrowser

* Adds medium sidebar test for filebrowser

* Fix loop issue

* Update comment

* Update Playwright Snapshots

* Reinstates previous screen shots not modified by this change

* Freezes dates at one day ago

* Disable stylelint for container queries

* Avoid using page in beforeAll

* Uploads files with time stamp override in a separate test pass

* Reverts screen shots not modified by this change

* Reverts tests so I can put them into another branch

* Reinstates screenshots without sample files

This reverts commit d682f4ab8d014653b5c1648f88aef2c3ed7f533b.

* Uses VirtualDOM.render()

* Reverts screen shot that this branch should not modify

* Updates sidebars test options to avoid timeouts

* Reinstates older screenshot

* Removes unused title parameter

* Updates comment about ES version

* Removes container queries, triple rendering; updates listings on resize

* Removes comment about container queries, no longer valid

* Only rerenders nodes if modified style has changed

* Only updates modified dates on resize

* Update visibility level of new member method

* Null check for container width

* Revisions per @krassowski

* Update packages/filebrowser/src/listing.ts

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

* Adds test case for string replacement that contains query string

* Revert "Adds test case for string replacement that contains query string"

This reverts commit fa849f0.

* Update Playwright Snapshots

* Update Playwright Snapshots

* Update example snapshot, pick up more stable versions of flaky snapshots

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants