-
-
Notifications
You must be signed in to change notification settings - Fork 590
feat: Attachment of Files to Documents using S3. #280
base: main
Are you sure you want to change the base?
Conversation
af511c6
to
757eded
Compare
757eded
to
bc94f6f
Compare
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. |
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.
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.
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. Markdown also allows for embedded html so that's another option. |
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.
|
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 |
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:
I think we are good to go for the development of the first version. |
Full Fledge File Upload to S3
I have pushed the code for file upload which replicates the image upload functionality for all file types. Features added.
Issues:
|
I don't think you need the |
Makes Sense @tommoor Thanks will make the changes. |
Issue: |
I'm afraid I don't know what you mean – make a gif? |
I think I got the issue!
I think I got the fix for it. |
Any thoughts on the PR? |
example/src/index.js
Outdated
// Delay to simulate time taken to upload | ||
return new Promise(resolve => { | ||
setTimeout( | ||
() => resolve("https://loremflickr.com/1000/1000"), |
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.
Probably best to use a non-image for this example, otherwise it's just confusing.
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.
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) |
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.
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.
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 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?
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.
Yes, here's the existing code:
rich-markdown-editor/src/components/LinkToolbar.tsx
Lines 71 to 88 in 7b03109
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="*" |
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.
Makes sense to make this a prop
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.
are you talking about the default accept prop?
src/index.tsx
Outdated
.file { | ||
a { | ||
pointer-events: ${props => (props.readOnly ? "initial" : "none")}; | ||
} | ||
} | ||
|
||
.file.placeholder { | ||
position: relative; | ||
background: ${props => props.theme.background}; | ||
|
||
a { | ||
opacity: 0.5; | ||
} | ||
} |
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.
This seems to be unused
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.
sure will remove this
src/lib/uploadFilePlaceholder.ts
Outdated
@@ -0,0 +1,51 @@ | |||
import { Plugin } from "prosemirror-state"; |
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.
The entire thing here seems to be unused now it's no longer based on an image?
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.
sure will remove this
{ | ||
name: "file", | ||
title: "File", | ||
icon: ImageIcon, |
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.
Going to need a unique icon here – there isn't a simple "file" icon in the set, but I can get one added.
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.
Yes please get it added will use the same Icon here
src/menus/block.ts
Outdated
name: "file", | ||
title: "File", | ||
icon: ImageIcon, | ||
keywords: "file doc pdf", |
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.
keywords: "file doc pdf", | |
keywords: "doc pdf", |
No need to include "file" as the name and title are included in the keywords automatically
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.
Okay got it
Hello 👋 Great work on outline and this, are there any news about this PR? |
Hey, I will be soon pushing the final code for the Feature.
…On Mon, 14 Dec, 2020, 7:57 pm Marco Nunnari, ***@***.***> wrote:
Hello 👋 Great work on outline and this, do we have any news about this
PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#280 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFQMDQLPE7GCVBBQSH6L5N3SUYOFNANCNFSM4RE2OJXQ>
.
|
@chanchadsahil7 any updates on this? Need any help💪 ? |
Thanks for sharing a great project. |
Honestly review was never requested again so it disappeared off my GitHub "reviews requested" list, definitely open to this. |
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:
Thanks for reviewing my first open-source PR🙂
cc: @chsasank