-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: file actions on dashboard list #680
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -92,6 +102,8 @@ export const router = createBrowserRouter( | |||
element={<CloudFilesMigration.Component />} | |||
loader={CloudFilesMigration.loader} | |||
/> | |||
|
|||
<Route path="/api/files/:uuid/sharing" action={shareFileMenuAction} loader={shareFileMenuLoader} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure what to call this route. These are called "resource routes" in Remix, but that's less of a thing in react-router because it's all client side.
The idea is: we need URLs that react-router fetchers can access without doing navigations. So it's sort of like a client-side, react-router specific API. So for now i'm just having these mirror the actual server API routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually need a whole route, or do you just need a fetcher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were fetching directly from the API with our fetchers, then we wouldn't need a route.
But we have the apiClient
for talking to the API, so we need client routes for the loaders/fetchers, and the client routes use the apiClient
to talk to the API. So in this case, yes we need it.
Maybe in the future we can hit the API directly from our fetchers, but I haven't really thought about how that changes the architecture of the current codebase. So this was the most direct method to get this working.
|
@davidkircos those have all been fixed (also, lmk what you think about the show/hide effect for each row item's icons, similar to google docs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments on the code. I am not sure if they marit any changes.
File-related actions from the dashboard
On desktop we surface a few of the actions:
And on smaller screens we hide all actions under the dropdown:
"Share File" menu re-used across dashboard and app
/file/:uuid/sharing
endpoint on-demand.Reusable "Dialog" component for dialogs
Share & feedback use the same underlying component for displaying their content:
Todos
updated_date
(see slack thread)<ShareFileMenu>
across the app (fetch its own data, or pass it in manually now...)