Skip to content

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

Merged
susnux merged 6 commits into
masterfrom
fix/files-navigation-not-active
Jan 25, 2024
Merged

fix(files): Make the navigation reactive to view changes and show also sub routes as active#42992
susnux merged 6 commits into
masterfrom
fix/files-navigation-not-active

Conversation

@susnux

@susnuxsusnux commented Jan 20, 2024

Copy link
Copy Markdown
Contributor

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

@susnuxsusnux added this to the Nextcloud 29 milestone Jan 20, 2024
@susnux

Copy link
Copy Markdown
ContributorAuthor

/backport to stable28

Comment threadapps/files/src/views/Navigation.vue Outdated
skjnldsv
skjnldsv previously requested changes Jan 21, 2024

@skjnldsvskjnldsv left a comment

Copy link
Copy Markdown
Member

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
susnuxforce-pushed the fix/files-navigation-not-active branch from 2458ca2 to 02cc1b4CompareJanuary 21, 2024 18:08
@susnux

susnux commented Jan 21, 2024

Copy link
Copy Markdown
ContributorAuthor

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 skjnldsvJanuary 21, 2024 18:10
Comment threadapps/files/lib/Controller/ViewController.php Fixed
Comment threadapps/files/lib/Controller/ViewController.php Fixed
@susnux
susnuxforce-pushed the fix/files-navigation-not-active branch from 98e6568 to 8767795CompareJanuary 21, 2024 19:08
@susnux

Copy link
Copy Markdown
ContributorAuthor

Looks like this now:

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

Comment threadapps/files/lib/Activity/Helper.php Outdated
@susnux
susnuxforce-pushed the fix/files-navigation-not-active branch from 8767795 to 77ea357CompareJanuary 22, 2024 14:16
@susnux
susnux requested a review from emoral435January 22, 2024 14:17

@emoral435emoral435 left a comment

Copy link
Copy Markdown
Contributor

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 :)

Comment threadapps/files/src/views/favorites.ts Outdated
Comment threadapps/files/src/views/Navigation.vue
Comment threadapps/files/src/views/Navigation.vue Outdated
@skjnldsv

skjnldsv commented Jan 24, 2024

Copy link
Copy Markdown
Member

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
susnuxforce-pushed the fix/files-navigation-not-active branch 2 times, most recently from 70b7ce7 to 9e786c5CompareJanuary 24, 2024 12:19
@susnux

Copy link
Copy Markdown
ContributorAuthor

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 reviewJanuary 24, 2024 12:21

Fixed the regression

@susnux
susnux requested a review from emoral435January 24, 2024 12:21
@skjnldsv

skjnldsv commented Jan 24, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

/backport to stable28

ShGKme

This comment was marked as resolved.

@susnux
susnuxforce-pushed the fix/files-navigation-not-active branch from 80c1961 to cdbc39bCompareJanuary 25, 2024 01:42
@susnuxsusnux 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 ShGKmeJanuary 25, 2024 02:08
@susnux
susnuxforce-pushed the fix/files-navigation-not-active branch from cdbc39b to 7ae29cfCompareJanuary 25, 2024 02:08

@ShGKmeShGKme left a comment

Copy link
Copy Markdown
Contributor

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 listClick from the files list
imageimage

@sorbaughsorbaugh mentioned this pull request Jan 25, 2024
@susnux
susnuxforce-pushed the fix/files-navigation-not-active branch 2 times, most recently from 27c5933 to 2db1440CompareJanuary 25, 2024 11:39
@susnux

Copy link
Copy Markdown
ContributorAuthor

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 ShGKmeJanuary 25, 2024 11:41
@ShGKme

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

susnuxand 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
susnuxforce-pushed the fix/files-navigation-not-active branch from 2db1440 to eea7f62CompareJanuary 25, 2024 14:09
@susnux

Copy link
Copy Markdown
ContributorAuthor

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

@ShGKmeShGKme left a comment

Copy link
Copy Markdown
Contributor

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 masterJan 25, 2024
@susnux
susnux deleted the fix/files-navigation-not-active branch January 25, 2024 15:47
@blizzzblizzz mentioned this pull request Mar 5, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@susnux@skjnldsv@ShGKme@github-advanced-security@emoral435@nextcloud-command