Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

feat: Attachment of Files to Documents using S3. #603

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FeralMib
Copy link

This is an extension of #280 including the original commit there, rebased and extended on current master.

For further details see the original PR.

I've added support for a basic placeholder during upload, added a suitable menu icon and fixed compatibility issues during during rebasing

The matching support in outline server can be found in outline/outline#2859

Copy link
Member

@tommoor tommoor left a comment

Choose a reason for hiding this comment

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

Apart from the placeholder issue outlined in this review I think really the question is whether a plain text link is good enough – and I'm tempted to say no.

We really want to maintain a semantic link to the fact that it's a file upload within the document structure by creating a file node type that stores the attachmentId etc, this will allow us to do much more interesting things in the future.

cc @thenanyu

for (const file of files) {

// Insert a placeholder link
var placeholder = `[${file.name} uploading...]`
Copy link
Member

Choose a reason for hiding this comment

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

This type of placeholder isn't sufficient as the text document can change while the file is uploading – in that case the text will be replaced incorrectly. Instead it should work like the image placeholder with a widget decoration that is then searched for again to find it's current position once the file upload is complete

// const phref = `#uploading_${file.name}`;
view.dispatch(
view.state.tr
.insertText(placeholder, from, to-1)
Copy link
Member

Choose a reason for hiding this comment

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

-1 is a bit strange here

{
name: "file",
title: dictionary.file,
icon: NotepadIcon,
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the DocumentIcon for now, it looks like a file

@warnus
Copy link

warnus commented Dec 22, 2021

Sorry about interrupt you guys. I really look forward to including this feature. If file upload using just link that I'm wondering that how to remove the file from storage.

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

Successfully merging this pull request may close these issues.

None yet

3 participants