Skip to content

fix(files): correct params on Go back link #43116

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.

Sign up for GitHub

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

Closed
ShGKme wants to merge 2 commits into from
Closed

fix(files): correct params on Go back link #43116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9396ad
fix(files): correct params on Go back link
ShGKme Jan 25, 2024
c2100c0
chore(assets): Recompile assets
ShGKme Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions apps/files/src/views/FilesList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@
data-cy-files-content-empty>
<template #action>
<NcButton v-if="dir !== '/'"
:aria-label="t('files', 'Go to the previous folder')"
type="primary"
:to="toPreviousDir">
{{ t('files', 'Go back') }}
Copy link
Member

@skjnldsv skjnldsv Jan 26, 2024

Choose a reason for hiding this comment

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

Suggested change
{{ t('files', 'Go back') }}
{{ t('files', 'Go to the parent folder') }}

Expand Down Expand Up @@ -341,7 +340,9 @@ export default defineComponent({
*/
toPreviousDir(): Route {
const dir = this.dir.split('/').slice(0, -1).join('/') || '/'
return { ...this.$route, query: { dir } }
const fileid = this.pathsStore.getPath(this.currentView?.id, dir)?.toString()
// @ts-expect-error Route expect params to be string, but we need to explicitly set undefined when there is no fileid
return { ...this.$route, params: { fileid }, query: { dir } }
Comment on lines +343 to +345
Copy link
Member

@skjnldsv skjnldsv Jan 26, 2024

Choose a reason for hiding this comment

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

You should not need to provide the fileid.
Maybe making sure we just keep the view?

Suggested change
const fileid = this.pathsStore.getPath(this.currentView?.id, dir)?.toString()
// @ts-expect-error Route expect params to be string, but we need to explicitly set undefined when there is no fileid
return { ...this.$route, params: { fileid }, query: { dir } }
return { name: this.$route.name, params: { view: this.$route.params?.view || 'files' }, query: { dir } }

Copy link
Contributor Author

@ShGKme ShGKme Jan 26, 2024

Choose a reason for hiding this comment

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

If I understand our files routing correctly, we always have fileid in the path, except root.

For example, if I open /Foo/Bar and Bar directory has fileid=700, then the current route is:

  • In Files: /apps/files/files/700?dir=/Foo/Bar
  • In favorites: /apps/files/favorites/700?dir=/Foo/Bar

Let's say, I'm in the root, then I'm going to Baz and back to Bar.

  1. /apps/files/files?dir=/
  2. /apps/files/files/699?dir=/Foo
  3. /apps/files/files/700?dir=/Foo/Bar
  4. /apps/files/files/701?dir=/Foo/Bar/Baz

From 4 shouldn't I return to 3 /apps/files/files/700?dir=/Foo/Bar?
Why /apps/files/files/?dir=/Foo/Bar?

If going back should be without fileid, does it mean that while going through directories forward and though breadcrumb there should also be no fileid?

  1. /apps/files/files?dir=/
  2. /apps/files/files?dir=/Foo
  3. /apps/files/files?dir=/Foo/Bar
  4. /apps/files/files?dir=/Foo/Bar/Baz

Copy link
Member

@skjnldsv skjnldsv Jan 26, 2024

Choose a reason for hiding this comment

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

we always have fileid in the path

no, having fileid in the path is optional, we don't rely on it to open folders or browse.
We only rely on it to scroll to a specific file in the current opened folder or trigger a file action on load

Copy link
Contributor Author

@ShGKme ShGKme Jan 26, 2024

Choose a reason for hiding this comment

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

Does it mean that links to folders in the files list should not contain fileid at all?
Because there is no such a file in the folder, nothing to scroll to.

For example, if /Foo folder has fileid=700, then path /apps/files/files/700?dir=/Foo doesn't make sense — there is no "700" in the /Foo, only in parent /.

Copy link
Member

@skjnldsv skjnldsv Jan 26, 2024

Choose a reason for hiding this comment

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

You can have it or you can't.
It will have no effect on the loading of the folder.

the fileid, like in older versions, is only used within the Files app to work with a file within a folder (like open the sidebar, etc etc)

Copy link
Member

@skjnldsv skjnldsv Jan 26, 2024 •
edited by ShGKme
Loading

Choose a reason for hiding this comment

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

  1. Just in the file list when we go to the next folder
  2. In the breadcrumbs
  3. In the "Go back" link
  1. Yes
  2. Yes
  3. aaaaand yes/no, it's good to have it but it's optional

We try to keep the current fileid for this specific reason: #40515

Copy link
Contributor Author

@ShGKme ShGKme Jan 26, 2024

Choose a reason for hiding this comment

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

  1. Yes
  2. Yes

will trigger the sidebar action for the file id

Does it mean that opening any folder is supposed to also open a sidebar after it?

If it is not, we have a inconsistent here:

  • Being on a page /:fileid doesn't open a sidebar
  • Opening a link with /:fileid opens a sidebar
  • => A fact of reloading a page opens a sidebar o_O

Copy link
Contributor Author

@ShGKme ShGKme Jan 26, 2024

Choose a reason for hiding this comment

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

Probably, there should be a query param for sidebar, like ?details, similar to openfile?

Copy link
Member

@skjnldsv skjnldsv Jan 27, 2024

Choose a reason for hiding this comment

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

No please 🙈
Let's not complicate URLs.

Being on a page /:fileid doesn't open a sidebar
Opening a link with /:fileid opens a sidebar
=> A fact of reloading a page opens a sidebar o_O

That's not a inconsistency, this is the feature. It's been long discussed and implemented by the design team.
On load, if you provide a fileid for a FILE, we open the sidebar (if screen size allows)

Copy link
Member

@skjnldsv skjnldsv Jan 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, please, let's not dive into this and reconsider 6 years of feature requests, this is not what this issue is about at all.
You have no idea the amount of use cases the Files app supports and I had to match up.

},

shareAttributes(): number[]|undefined {
Expand Down Expand Up @@ -567,7 +568,7 @@ export default defineComponent({
* Refreshes the current folder on update.
*
* @param {Node} node is the file/folder being updated.
*/
*/
onUpdatedNode(node) {
if (node?.fileid === this.currentFolder?.fileid) {
this.fetchContent()
Expand Down
4 changes: 2 additions & 2 deletions dist/files-main.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/files-main.js.map
View file
Open in desktop

Large diffs are not rendered by default.