-
Notifications
You must be signed in to change notification settings - Fork 76
feat: Create file storage file view #4767
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
Conversation
c986317
to
d9eaf18
Compare
🤖 This is an automated code coverage reportTotal coverage (lines): 29.05% |
d9eaf18
to
afc5964
Compare
Signed-off-by: Andrew Burnes <[email protected]>
afc5964
to
e896f37
Compare
This loads the file details if you're already within a folder a lot slower, because it's a separate view/query, which makes sense but feels like a regression. I expect most of the time people load a file, it will be because they're in a folder, and only rarely will their first visit/page be to this route. What do we get for this besides fixing the red error flash in the current experience? what are the other gains? |
It fixes the flash error, improves maintainability, and allows future features with more functionality and data. When we add file history, that will add additional data to every file in the paginated query which would add to slower responses. If we add file viewing or update operations, the seperate page be easier to maintain. |
i agree that adding the file history or base64encoded file contents, etc, to the file list response for every file would be heavy, but I think we could still seed the detail view with the known data from the array of files response, and then follow up with this secondary data if/when we need it in another response. In this proposed change, although we already have all the relevant data loaded for this file if you're in a directory with it, when someone clicks a file, we ask the API for it again just because its in a new route. What's in main currently doesn't require an additional request, which feels a lot faster. I think that most of the time people look at a file's details, they're going to be in a directory already; very rarely will they "land" on this file ID view via a link from a colleague or bookmark. It seems like we're optimizing for matching the shape of the backend API, rather than the actual likely UX. maybe we could pass in the existing known data as initialData in the ReactQuery hook? |
Signed-off-by: Andrew Burnes <[email protected]>
2839f20
to
50cbbc1
Compare
Closed in favor of #4779 |
Closes #2372 #4752 #4760
Changes proposed in this pull request:
security considerations
None