Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

apburnes
Copy link
Contributor

@apburnes apburnes commented Mar 27, 2025

Closes #2372 #4752 #4760

Changes proposed in this pull request:

  • Creates a file storage file view page
  • Adds liveDomain to the site response
  • Fixes bug in location bar grandparent directory path

security considerations

None

@apburnes apburnes force-pushed the feat-create-file-storage-file-page branch from c986317 to d9eaf18 Compare March 27, 2025 20:42
@cloud-gov-pages-operations
Copy link
Contributor

cloud-gov-pages-operations commented Mar 27, 2025

🤖 This is an automated code coverage report

Total coverage (lines): 29.05%
Coverage diff: 0.79% 📈

@apburnes apburnes requested a review from a team March 27, 2025 23:15
drewbo
drewbo previously approved these changes Mar 28, 2025
@apburnes apburnes force-pushed the feat-create-file-storage-file-page branch from d9eaf18 to afc5964 Compare March 28, 2025 19:02
@apburnes apburnes force-pushed the feat-create-file-storage-file-page branch from afc5964 to e896f37 Compare March 28, 2025 19:55
@sknep
Copy link
Contributor

sknep commented Mar 28, 2025

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?

@apburnes
Copy link
Contributor Author

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.

@sknep
Copy link
Contributor

sknep commented Apr 1, 2025

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?

@apburnes apburnes force-pushed the feat-create-file-storage-file-page branch from 2839f20 to 50cbbc1 Compare April 1, 2025 21:08
@apburnes
Copy link
Contributor Author

apburnes commented Apr 8, 2025

Closed in favor of #4779

@apburnes apburnes closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants