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

feat: File Browser Update (urls, examples, etc) #308

Merged
merged 123 commits into from
Apr 3, 2023
Merged

Conversation

davidkircos
Copy link
Collaborator

@davidkircos davidkircos commented Feb 23, 2023

Resolves #323

Functionality / things to test:

  • Routing:
  • File menu:
    • Opens with: CMD + O, Toolbar "File -> Open...", or Command Palette "File: Open..."
    • Close with: ESC or X in UI
    • Create a new file (names it "Untitled", goes to origin, zooms to 100%)
    • Open an example file (names it file name, goes to origin, zooms to 100%)
    • Open a file from local device (names it based on file on disk sans extension, goes to origin, zooms to 100%)
      • Make sure to test this with an old file (something from the current version of production)
    • Open a file from a URL (names based on file path in, goes to origin, zooms to 100%)
    • Delete an in-memory file (deletes it from UI and indexDB)
    • Download a local copy of an in-memory file
  • "File -> Save local copy" (from toolbar or command palette) downloads file with current name + .grid extension
  • "File -> New" (from toolbar or command palette) starts a new file, zoomed to 100% at (0,0)
  • In-app link to "Files" page in docs works
  • Can rename a file from the top bar (empty filenames or filenames with only spaces are not allowed)
  • "Open file" menu accessible from tablet/mobile
  • Files you currently have in memory are migrated to our new storage mechanism
    • You can test this locally by running main on localhost:3000 and create a couple different files in memory, then checkout this branch and load the app and the files you created should be accessible in the file menu

Todos:

  • Current files in memory should survive this new change when it merges
  • Write docs for how files/file formats work in Quadratic (code currently points at docs.quadratichq.com/files)
  • Fix contextual TODOs in code
  • Firing twice on each save (appears to be something with pixiapp, not new code)
  • Support old files
  • Fix/update sample files
    • Update with version "1.1" meta info?
    • Some aren't loading right now?
    • Updates from main?
  • Add filename to document.title {File Name} - Quadratic
  • File issue for after merge: open separate PR to use context on app and sheetConroller

Nice to haves:

  • Example files that, when you clone them, appear with the same name as in the example file list
  • Weird flicker when sheet loads first, then open menu renders
    • Coming from how the app loads. useLocalFiles hook has an async action to load the file from memory. Would require re-architecting how this works...

Leaving out:

@aws-amplify-us-west-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-308.de5w5iglj13on.amplifyapp.com

@davidkircos davidkircos changed the title open file from url feat: File Browser Update (urls, examples, etc) Mar 1, 2023
@jimniels jimniels marked this pull request as ready for review March 31, 2023 21:09
@davidkircos
Copy link
Collaborator Author

When processing old files to new the order is reversed: (not a big deal)
CleanShot 2023-03-31 at 20 00 04@2x
screencapture-localhost-3000-2023-03-31-20_06_02

Have you seen this "SCROLLABLE"? thing in the console before?
CleanShot 2023-03-31 at 20 11 06@2x

What do you think about chaing the Quadratic menu to have a "back to files" option at the top the way Figma does?
CleanShot 2023-03-31 at 20 11 59@2x
In Sheets the top left icon takes to back to your files list
CleanShot 2023-03-31 at 20 13 44@2x

Still testing file actions, but haven't noticed any problems so far. @jimniels

@davidkircos
Copy link
Collaborator Author

Visual overflow on mobile image

@HactarCE
Copy link
Collaborator

HactarCE commented Apr 3, 2023

Ctrl + O on Windows opens the browser popup. I'm not sure if it's possible for us to override that.

@HactarCE
Copy link
Collaborator

HactarCE commented Apr 3, 2023

  • Also, ?local=... on Firefox always opens the file menu with no error.
  • Somehow by refreshing with the right timing using ?local=<invalid> on Chrome, I managed to completely erase all the local files (!!!) but I can't reproduce this. Not sure how that happened.
  • It would be nice if dragging a .grid file in loaded it instead of trying to parse a CSV

@jimniels
Copy link
Collaborator

jimniels commented Apr 3, 2023

When processing old files to new the order is reversed

✅ Fixed in 242a019

Have you seen this "SCROLLABLE"? thing in the console before?

I have not seen that. Was that on the preview branch environment?

What do you think about chaing the Quadratic menu to have a "back to files" option at the top

✅ I like it. Updated.

CleanShot 2023-04-03 at 11 40 02@2x

Visual overflow on mobile

Fixed. Due to the responsive nature of the screen, I went with a background on both those items, so now on mobile they float over any content that scrolls under them. It could use more refinment, but it’s the quickest way to get responsive on this and i’m happy enough with it.

CleanShot 2023-04-03 at 11 59 14@2x

@jimniels
Copy link
Collaborator

jimniels commented Apr 3, 2023

@HactarCE

Ctrl + O on Windows opens the browser popup. I'm not sure if it's possible for us to override that.

It should be possible, we're overriding it in the other browsers. Not sure why this code isn't working on windows but it is on Mac.

However, that said, CMD + O was not implemented as a toggle. It was one-way, e.g. only for opening the "File menu" not closing it. I've tweaked that. Now CMD + O should toggle the file menu's visibility, e.g. opens it if it's not visible and closes it if it's visible except in the case where you don't have an actively open file in the app in which case it does nothing (because there's nothing to close and go to).

Also, ?local=... on Firefox always opens the file menu with no error.

Hm...I can't reproduce this on Firefox for Mac.

Loading a file using ?local=<valid-id> loads the file:

CleanShot 2023-04-03 at 12 13 37@2x

Whereas loading a file using ?local=<invalid-id> (e.g. /?local=abcd) can't load the file and opens the file menu with an error.

CleanShot 2023-04-03 at 12 14 06@2x

Somehow by refreshing with the right timing using ?local= on Chrome, I managed to completely erase all the local files (!!!) but I can't reproduce this. Not sure how that happened.

This one makes me nervous, but I haven't been able to reproduce it.

I did add a check in the code that will only delete stuff when updating old files if it successfully imports it.

Again, i'm not sure how this was triggered. Delete actions are only accessible in the code when triggered by a user action and confirmation via the UI. The other deletes happen only on migration files. So i'm not sure how this even happened...?

It would be nice if dragging a .grid file in loaded it instead of trying to parse a CSV

Agree. I was actually wanting to do that while I was working on this branch. But figured we'd push that for a later feature. I opened an issue for it.

@jimniels
Copy link
Collaborator

jimniels commented Apr 3, 2023

@davidkircos I added a check to the code that makes sure we only delete a file in memory when we are sure it's been imported/migrated to this new way of saving/storing files.

However, it is worth noting that if a file in memory fails to migrate/import, that fails silently to the user (there's an error logged to the console). So, right now, that file just stays there in localstorage but is inaccessible to the user via the application UI. We don't tell them one of their files failed to import and there's no UI for trying to retrieve it somehow. This is less than ideal but i'm not really sure how much it happens? Maybe we should log this as a specific event in sentry just to see if it happens a lot?

Copy link
Collaborator

@jimniels jimniels left a comment

Choose a reason for hiding this comment

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

Gonna go ahead and approve this PR

CleanShot 2023-04-03 at 16 45 33@2x

@jimniels jimniels merged commit 135e1a5 into main Apr 3, 2023
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.

feat: working in multiple tabs/windows
4 participants