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

fix(files): Make the navigation reactive to view changes and show also sub routes as active #42992

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 20, 2024

Summary

This resolves two issues with the files app navigation:

  1. The navigation was not reactive as the currentView is a computed value but all source objects are not reactive (@nextcloud/files Navigation). So to ensure changes to the navigation can be detected the object is made observable.
  2. Removed the exact prop on the app navigation as otherwise it will not show all files active if you visited a file like apps/files/files/123.
  3. Minor type fixes in Navigation.vue

Screenshots

Before

Pay attention that while the navigation changes the file list does not. Also not that no navigation is active within a subfolder.

vokoscreenNG-2024-01-21_00-54-00.mp4

After

See that navigation works and the file list is updated + also in subfolders the navigation is kept active.

Please note: The current version (video) is shown here: #42992 (comment) )

vokoscreenNG-2024-01-21_00-52-54.mp4

Checklist

@susnux susnux added this to the Nextcloud 29 milestone Jan 20, 2024
@susnux
Copy link
Contributor Author

susnux commented Jan 20, 2024

/backport to stable28

@susnux susnux self-assigned this Jan 21, 2024
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Removed the exact prop on the app navigation as otherwise it will not show all files active if you visited a file like apps/files/files/123.

Now it does this, which is a regression.
I think we need our custom exact checker 🤔
image

@susnux susnux force-pushed the fix/files-navigation-not-active branch from 2458ca2 to 02cc1b4 Compare January 21, 2024 18:08
@susnux
Copy link
Contributor Author

susnux commented Jan 21, 2024

Now it does this, which is a regression.
I think we need our custom exact checker 🤔

I do not think we need such a complex solution, I think we can go for the moment with just checking the view we are in.
This fixes the initial issue and will not lead to that regression.

I pushed a fix that only disables exact for the files view.

We might later add a exact property to the View entry itself to let them decide, but thats up for discussion in a future iteration(?).

Edit we have another issue with the favorites, the generated route on the navigation did not include the fileid but it was set when clicking the file from the filelist.
So the latest commit fixes this by also settings the fileid param on the route like the filelist does on click (this is also more consistent with the other behavior in files).

@susnux susnux requested a review from skjnldsv January 21, 2024 18:10
@susnux susnux force-pushed the fix/files-navigation-not-active branch from 98e6568 to 8767795 Compare January 21, 2024 19:08
@susnux
Copy link
Contributor Author

susnux commented Jan 21, 2024

Looks like this now:

vokoscreenNG-2024-01-21_20-07-00.mp4

@susnux susnux force-pushed the fix/files-navigation-not-active branch from 8767795 to 77ea357 Compare January 22, 2024 14:16
@susnux susnux requested a review from emoral435 January 22, 2024 14:17
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Overall, love this PR. I'm just getting assigned to more files-related issues for the spring sprint, and my advice was to ping you whenever I have a question or draft a PR, so seeing your code genuinely supports my growth here, and I will def ping you to review my PR's because the code quality here is really good. Just a few comments and nitpicks and this is perfect, I tried it on my local dev env and it also works :)

apps/files/src/views/favorites.ts Outdated Show resolved Hide resolved
apps/files/src/views/Navigation.vue Show resolved Hide resolved
apps/files/src/views/Navigation.vue Outdated Show resolved Hide resolved
@skjnldsv
Copy link
Member

skjnldsv commented Jan 24, 2024

What I don't get is that it worked before.
v28.0.0 and v28.0.1 doesn't have that reactivity issue.

What broke it in between?

EDIT: git bisect says #42994 broke it
EDIT2: yep, if I revert efaf1ce, it works again

@susnux susnux force-pushed the fix/files-navigation-not-active branch 2 times, most recently from 70b7ce7 to 9e786c5 Compare January 24, 2024 12:19
@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2024

What I don't get is that it worked before.

I wonder how it worked before because that object was never made reactive?

@susnux susnux dismissed skjnldsv’s stale review January 24, 2024 12:21

Fixed the regression

@susnux susnux requested a review from emoral435 January 24, 2024 12:21
@skjnldsv
Copy link
Member

skjnldsv commented Jan 24, 2024

I wonder how it worked before because that object was never made reactive?

It's a value from an object, which is passed as a reference, it should be reactive by default.
Something changed with the 2.7.16 update, feel free to try it yourself 🤷

EDIT: this is actually not an issue with vue, but the 5.1.0 release of @nextcloud/dialogs
Downgrading to 5.0.3 fixes the reactivity issue

EDIT2: wrong test, my bad, downgrading vue to 2.7.15 does fix it

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2024

/backport to stable28

ShGKme

This comment was marked as resolved.

@susnux susnux force-pushed the fix/files-navigation-not-active branch from 80c1961 to cdbc39b Compare January 25, 2024 01:42
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 25, 2024
@susnux

This comment was marked as resolved.

@susnux susnux requested a review from ShGKme January 25, 2024 02:08
@susnux susnux force-pushed the fix/files-navigation-not-active branch from cdbc39b to 7ae29cf Compare January 25, 2024 02:08
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I have /foo/bar/ both favorite. Initially "All favorites" is open

image

Depending on how I go to the bar, I have different result and selected button on the navigation.

If I go from the files list (All favorites is open), I see no content.

Click from the favorites list Click from the files list
image image

@sorbaugh sorbaugh mentioned this pull request Jan 25, 2024
@susnux susnux force-pushed the fix/files-navigation-not-active branch 2 times, most recently from 27c5933 to 2db1440 Compare January 25, 2024 11:39
@susnux
Copy link
Contributor Author

susnux commented Jan 25, 2024

Depending on how I go to the bar, I have different result and selected button on the navigation.

Resolved this

vokoscreenNG-2024-01-25_12-39-35.mp4

@susnux susnux requested a review from ShGKme January 25, 2024 11:41
@ShGKme
Copy link
Contributor

ShGKme commented Jan 25, 2024

Resolved this

It still doesn't fully work for me. It fixes the navigation active, but the content is not displayed.

Try to add some content to the "Bar" folder. It must be listed then Favorites/Bar is open from the navigation.

@susnux
Copy link
Contributor Author

susnux commented Jan 25, 2024

I think it is unrelated, but another bug. This also happens on master, the problem here is that dir is set to /bar while it should be /foo/bar

susnux and others added 6 commits January 25, 2024 15:07
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…ion entries

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@susnux susnux force-pushed the fix/files-navigation-not-active branch from 2db1440 to eea7f62 Compare January 25, 2024 14:09
@susnux
Copy link
Contributor Author

susnux commented Jan 25, 2024

@ShGKme pushed a commit that fixes also that issue, you can try again :)

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Works now 🥳

@susnux susnux merged commit 78cc1d2 into master Jan 25, 2024
54 checks passed
@susnux susnux deleted the fix/files-navigation-not-active branch January 25, 2024 15:47
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants