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. #280

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

Conversation

chanchadsahil7
Copy link

@chanchadsahil7 chanchadsahil7 commented Sep 10, 2020

Background:
At Qure.ai we have been using the self-hosted outline and its feature for maintaining our internal documentation and knowledge base.

Problem:
We have been having a hard time uploading and maintaining files except for images on the outline that we are using.

Solution:

  • So we have built a draft of the files attachment using the original image upload feature and supported all kinds of files to be upload in the outline.
  • We understand that this isssue has been already requested.
  • The above issue requested Google Drive and Dropbox integration. However, we believe it is not a great direction to integrate third party for a feature as simple as file upload.
  • S3 is a files service provider over the cloud and is already an integral part of the outline service.
  • So we thought why not use the already existing S3 upload integration and support all kinds of files to be stored inside a document.

Thanks for reviewing my first open-source PR🙂

cc: @chsasank

@chanchadsahil7 chanchadsahil7 changed the title Attachment Files : S3 Bucket Attachment of Files to Documents using S3. Sep 10, 2020
@tommoor
Copy link
Member

tommoor commented Sep 14, 2020

Hey @chanchadsahil7 – this is interesting and I'm glad you were able to fork to handle the functionality you needed. As it stands I don't think we'll be able to merge this one as it's kind of "abusing" the markdown image syntax to host files – we'd need a new way of representing files in the markdown that maintains the current image behavior.

@chsasank
Copy link

chsasank commented Sep 14, 2020

As it stands I don't think we'll be able to merge this one

Of course, this is only a proof of concept. We wanted to get feedback early as we develop so that the PR has higher chance of merging. We didn't want to complete the feature in a way that's not in line with your vision.

it's kind of "abusing" the markdown image syntax to host files

We used this to get a quick and dirty patch to demonstrate the feature. We mean to replace it with better code following your guidance.

we'd need a new way of representing files in the markdown

Indeed! How about GitHub's way of handling files as links? We can use some metadata to differentiate the files from links. We can also render the files the slack way.

image

Markdown also allows for embedded html so that's another option.

@chanchadsahil7
Copy link
Author

chanchadsahil7 commented Sep 14, 2020

Hey @tommoor thanks for the feedback but as it stands this was just for the POC and not to disturb the current markdown images syntax to host files.

  • The idea is to create a similar functionality as markdown image with a file doc node in the markdown so that whenever a file is attached to the editor it creates a link to the doc uploaded.
  • Attaching the idea of the file doc node.

Screenshot 2020-09-13 at 9 56 19 PM

@tommoor
Copy link
Member

tommoor commented Sep 16, 2020

Indeed! How about GitHub's way of handling files as links? We can use some metadata to differentiate the files from links. We can also render the files the slack way.

As links makes sense for a first version both for storage in the markdown and the visual display. Id suggest adding a callback with the same signature as the image callback and only show the option in the block menu when the callback is passed in. That way users of the library can opt in to handling file uploads.

We should also provide a prop that allows setting the accepted file types

@chanchadsahil7
Copy link
Author

I am glad that we are on the same page for the first version.

Below are the points that will be taken into consideration for this feature:

  • Build a file drop option on the block menu.
  • Upload files to S3 bucket.
  • Display files as links in the document.
  • Pass a callback similar to upload the image and have a prop to validate the file upload option to be displayed inside the block menu.

@tommoor @chsasank

I think we are good to go for the development of the first version.

@chanchadsahil7
Copy link
Author

@tommoor @chsasank

I have pushed the code for file upload which replicates the image upload functionality for all file types.

Features added.

  • Added Upload file callback.
  • Added Insert all files function.
  • Added a file node similar to the image node which adds files as a link in the doc.

Issues:

  • File node-link getting converted to an image node-link after saving the document.

@tommoor tommoor changed the title Attachment of Files to Documents using S3. feat: Attachment of Files to Documents using S3. Oct 7, 2020
@tommoor
Copy link
Member

tommoor commented Oct 7, 2020

File node-link getting converted to an image node-link after saving the document.

I don't think you need the FileDoc component at all, just insert the uploaded url as a regular link and store it as a markdown link instead of a markdown image. Would this work for your usecase?

@chanchadsahil7
Copy link
Author

File node-link getting converted to an image node-link after saving the document.

I don't think you need the FileDoc component at all, just insert the uploaded url as a regular link and store it as a markdown link instead of a markdown image. Would this work for your usecase?

Makes Sense @tommoor Thanks will make the changes.

@chanchadsahil7
Copy link
Author

chanchadsahil7 commented Oct 7, 2020

@tommoor

  • Used the markdown link for the file upload and showing in the document.
  • Removed the file doc node that I created previously and used the existing createAndInsertLink function for inserting the link to the file.

Issue:
After the file is created the href attr inside the link is wrongly parsing the URL of the file and creating a document as a draft instead of just giving a file URL as output.

@tommoor
Copy link
Member

tommoor commented Oct 8, 2020

the href attr inside the link is wrongly parsing the URL of the file and creating a document as a draft

I'm afraid I don't know what you mean – make a gif?

@chanchadsahil7
Copy link
Author

I think I got the issue!

the href attr inside the link is wrongly parsing the URL of the file and creating a document as a draft

I'm afraid I don't know what you mean – make a gif?

I think I got the fix for it.

@chanchadsahil7
Copy link
Author

Any thoughts on the PR?

// Delay to simulate time taken to upload
return new Promise(resolve => {
setTimeout(
() => resolve("https://loremflickr.com/1000/1000"),
Copy link
Member

Choose a reason for hiding this comment

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

Probably best to use a non-image for this example, otherwise it's just confusing.

Copy link
Author

Choose a reason for hiding this comment

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

yep got it.

// start uploading the file file to the server. Using "then" syntax
// to allow all placeholders to be entered at once with the uploads
// happening in the background in parallel.
uploadFile(file)
Copy link
Member

Choose a reason for hiding this comment

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

The delay between the user starting the upload and the files being inserted will result in strange and potentially broken scenarios – the document content can have changed substantially in that time.

With the image extension a placeholder widget is used to hold the position for this reason, but a better comparison might be the logic when creating a document from the link menu – a "placeholder" link is created and then updated when the document has been successfully created. A similar approach seems necessary here.

Copy link
Author

Choose a reason for hiding this comment

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

let me know if I got this right:

You are suggesting that as soon as a user selects a file using the menu option you want me to insert a place holder to the document and save it. And then start the file upload process. Once the file is uploaded just replace the link.

is that what you are suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, here's the existing code:

const href = `creating#${title}…`;
// Insert a placeholder link
dispatch(
view.state.tr
.insertText(title, from, to)
.addMark(
from,
to + title.length,
state.schema.marks.link.create({ href })
)
);
createAndInsertLink(view, title, href, {
onCreateLink,
onShowToast,
dictionary,
});

A "placeholder" link is created with a temporary url while the document is being created. Once it's uploaded the href is swapped out. We could also target the temporary link in css to maybe fade it out or similar while the upload is happening.

type="file"
ref={this.fileInputRef}
onChange={this.handleFilePicked}
accept="*"
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to make this a prop

Copy link
Author

Choose a reason for hiding this comment

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

are you talking about the default accept prop?

src/index.tsx Outdated
Comment on lines 710 to 723
.file {
a {
pointer-events: ${props => (props.readOnly ? "initial" : "none")};
}
}

.file.placeholder {
position: relative;
background: ${props => props.theme.background};

a {
opacity: 0.5;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be unused

Copy link
Author

Choose a reason for hiding this comment

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

sure will remove this

@@ -0,0 +1,51 @@
import { Plugin } from "prosemirror-state";
Copy link
Member

Choose a reason for hiding this comment

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

The entire thing here seems to be unused now it's no longer based on an image?

Copy link
Author

Choose a reason for hiding this comment

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

sure will remove this

{
name: "file",
title: "File",
icon: ImageIcon,
Copy link
Member

Choose a reason for hiding this comment

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

Going to need a unique icon here – there isn't a simple "file" icon in the set, but I can get one added.

Copy link
Author

Choose a reason for hiding this comment

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

Yes please get it added will use the same Icon here

name: "file",
title: "File",
icon: ImageIcon,
keywords: "file doc pdf",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
keywords: "file doc pdf",
keywords: "doc pdf",

No need to include "file" as the name and title are included in the keywords automatically

Copy link
Author

Choose a reason for hiding this comment

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

Okay got it

@marconunnari
Copy link

marconunnari commented Dec 14, 2020

Hello 👋 Great work on outline and this, are there any news about this PR?

@chanchadsahil7
Copy link
Author

chanchadsahil7 commented Dec 14, 2020 via email

Base automatically changed from develop to main February 3, 2021 16:49
@malekhijazi
Copy link

@chanchadsahil7 any updates on this? Need any help💪 ?

@warnus
Copy link

warnus commented Aug 13, 2021

Thanks for sharing a great project.
Do you have any plans to merge this PR?

@tommoor
Copy link
Member

tommoor commented Aug 13, 2021

Honestly review was never requested again so it disappeared off my GitHub "reviews requested" list, definitely open to this.

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.

6 participants