Skip to content
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

Open recent files from within a sheet #2054

Open
wants to merge 5 commits into
base: qa
Choose a base branch
from
Open

Open recent files from within a sheet #2054

wants to merge 5 commits into from

Conversation

davidfig
Copy link
Collaborator

@davidfig davidfig commented Nov 9, 2024

This adds an Open recent file > submenu to the file menu. This stores the local file list in local storage.

  • add Open recent file submenu
  • add file that opens successfully to the top of the recent list
  • remove file that fails to open from the recent list
  • rename file updates the recent list
  • delete file removes the file from the recent list

Future:

  • Include the team name if it's a team file (this requires an API change)
  • Move from localStorage to DB storage, so a user always has their latest files regardless of what computer they're using
  • Add a normal Open... option to the file menu so you can get a list of files w/o returning to the dashboard

Copy link

qa-wolf bot commented Nov 9, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@cla-bot cla-bot bot added the cla-signed label Nov 9, 2024
Copy link

vercel bot commented Nov 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
quadratic ✅ Ready (Inspect) Visit Preview Nov 19, 2024 6:30pm

@davidfig davidfig added prototype exploring an idea that can be played with and removed cla-signed labels Nov 9, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-2054 November 9, 2024 17:24 Inactive
Copy link

codecov bot commented Nov 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.82%. Comparing base (e3dad46) to head (e0d70eb).

Additional details and impacted files
@@           Coverage Diff           @@
##               qa    #2054   +/-   ##
=======================================
  Coverage   90.82%   90.82%           
=======================================
  Files         260      260           
  Lines       57534    57534           
=======================================
  Hits        52257    52257           
  Misses       5277     5277           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cla-bot cla-bot bot added the cla-signed label Nov 10, 2024
@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-2054 November 10, 2024 15:44 Inactive
@davidfig davidfig linked an issue Nov 10, 2024 that may be closed by this pull request
@davidkircos
Copy link
Collaborator

Recent files seems like it should come from the API

@davidkircos davidkircos temporarily deployed to quadratic-api-dev-pr-2054 November 12, 2024 13:23 Inactive
@davidfig
Copy link
Collaborator Author

Agreed. But for a v1, this seems to work w/o having to change APIs.

@jimniels jimniels temporarily deployed to quadratic-api-dev-pr-2054 November 19, 2024 18:05 Inactive
@@ -101,6 +102,7 @@ export const deleteFile = {
try {
const data = getActionFileDelete({ userEmail, redirect });
submit(data, { method: 'POST', action: ROUTES.API.FILE(uuid), encType: 'application/json' });
updateRecentFiles(uuid, '', false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should happen in quadratic-client/src/routes/api.files.$uuid.ts in the delete action

If you put it in this file, then if a user deletes a file from the dashboard you won't see that reflected in your files list.

export const clearRecentFiles = () => {
localStorage.removeItem(RECENT_FILES_KEY);
window.dispatchEvent(new Event('local-storage'));
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this file should be put in quadratic-client/src/shared somewhere, because its functionality that really is shared between the app and the dashboard, because if you delete a file on the dashboard you'll want this code to run (which is what this comment is about).

So this should live outside of src/app since it's not just specific to the app, but shared between the app and the dashboard.

@@ -126,12 +127,14 @@ export function FilesListItemUserFile({
// Update on the server and optimistically in the UI
const data: FileAction['request.rename'] = { action: 'rename', name: value };
fetcherRename.submit(data, fetcherSubmitOpts);
updateRecentFiles(uuid, value, true, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar feedback here, if you move these into the routes file, you won't have to find every place where you rename or delete a file across both the dashboard and the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Recent Files List from sheet
3 participants