-
-
Notifications
You must be signed in to change notification settings - Fork 406
Adds download/upload attachment archive UIs #1551
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
Spoffy
wants to merge
38
commits into
main
Choose a base branch
from
spoffy/archive-ui
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
ab5cf00
Improves error handling for archives
Spoffy 44ec8f1
Cleans zip archive handling and removes async requirement
Spoffy 22c61a4
Updates archive tests
Spoffy b6787f4
Tidies imports
Spoffy d471af8
Adds missing res.end()
Spoffy 310f0e4
Adds download button for archives
Spoffy db00fb6
Extracts uploadFormData from uploadFiles
Spoffy a11a2f8
Adds archive upload UI
Spoffy ece3119
Adds archive upload UI
Spoffy e2c7a02
Fixes compile-time error caused by import
Spoffy 970002f
Improves attachment upload notification phrasing
Spoffy 006a532
Updates attachments downloads to new design
Spoffy 1cbead6
Updates copy menu designs + tests
Spoffy 1857f7d
Iterates on attachment download modal
Spoffy 714c645
WIP
Spoffy 06799d9
Adds upload tests
Spoffy a61d9d2
Adds test for re-upload warning
Spoffy b2e41c8
Fixes linter failure
Spoffy 09feac2
Updates to latest Figma design
Spoffy ee3db1e
Merge branch 'main' into spoffy/archive-ui
Spoffy ab72147
Fixes tests
Spoffy 94c3239
Removes lingering warning message
Spoffy 80bf482
Attempts fix for CI failure
Spoffy 414c922
Makes reconnected toast dismissable, fixes test
Spoffy b306297
Prevents download link changing page
Spoffy e15f0ae
Changes file format select style
Spoffy 7314244
Improves error case on chrome
Spoffy 106f686
Improves test with extra assert
Spoffy 808745f
Cleans import ordering
Spoffy 45b004e
Removes unused tooltip
Spoffy 11e5397
Fixes indentation
Spoffy 46691bc
Clarifies comment on archive upload method
Spoffy d615998
Changes to a less-hacky method of eager wrapping
Spoffy ed65e9a
Merge branch 'main' into spoffy/archive-ui
Spoffy f48dd7c
Fix condition for .tar download prompt showing
Spoffy 697dc3a
Removes export from uploadFormData
Spoffy 0407dc4
Guard against setting a possibly disposed observable
Spoffy edc597f
Removes dead commented code
Spoffy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In the presence of the comment above, I think it is better to test if the owner is not disposed after promise returns? (Here and below).
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 sure what you mean, can you describe the change in more detail?
Are you suggesting we check if the owner is disposed before setting the observable? If so, for my benefit learning GrainJS, what's the reason to do that?
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.
You are doing an async operation. By the time the promise is resolved, someone could move to another page, destroying all observables you are setting.
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've pushed some guards for this. Is this something that Observables should be handling internally?
It feels like once disposed, they should either error or do nothing if
set
is called on them.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.
Let me know if the additional checks are sufficient!