fix(files): correct params on Go back link#43116
Conversation
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
198d5e5 to
c2100c0
Compare
JuliaKirschenheuter
left a comment
There was a problem hiding this comment.
works, great!
| :aria-label="t('files', 'Go to the previous folder')" | ||
| type="primary" | ||
| :to="toPreviousDir"> | ||
| {{ t('files', 'Go back') }} |
There was a problem hiding this comment.
| {{ t('files', 'Go back') }} | |
| {{ t('files', 'Go to the parent folder') }} |
| 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 } } |
There was a problem hiding this comment.
You should not need to provide the fileid.
Maybe making sure we just keep the view?
| 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 } } |
There was a problem hiding this comment.
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.
/apps/files/files?dir=//apps/files/files/699?dir=/Foo/apps/files/files/700?dir=/Foo/Bar/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?
/apps/files/files?dir=//apps/files/files?dir=/Foo/apps/files/files?dir=/Foo/Bar/apps/files/files?dir=/Foo/Bar/Baz
There was a problem hiding this comment.
we always have
fileidin thepath
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
There was a problem hiding this comment.
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 /.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
- Just in the file list when we go to the next folder
- In the breadcrumbs
- In the "Go back" link
- Yes
- Yes
- 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
There was a problem hiding this comment.
- Yes
- 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
/:fileiddoesn't open a sidebar - Opening a link with
/:fileidopens a sidebar - => A fact of reloading a page opens a sidebar o_O
There was a problem hiding this comment.
Probably, there should be a query param for sidebar, like ?details, similar to openfile?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
Closed in flavor of #43174 |
Summary
Before, going back results an invalid URL. Updating the page results in infinity loop.
(Video with sound)
Before.mp4
This issue fixes the link. But probably we should also fix such an invalid link handling.
After-1.mp4
Also works on root.
After-2.mp4
TODO
aria-label- it is not correct to havearia-labeldifferent from a button text and not needed when there is a textfileidon"Go back"Checklist