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

Allow user to view attachments and clear unused attachments #1

Merged
merged 4 commits into from
Aug 28, 2019

Conversation

ZeroX-DG
Copy link

Hi @ehhc

I have added a section in setting to allow user to view attachments and clear unused attachments.

Feel free to tell me if you need me to fix something. Thanks 👍

@@ -624,6 +624,52 @@ function deleteAttachmentsNotPresentInNote (markdownContent, storageKey, noteKey
}
}

function getAttachments (markdownContent, storageKey, noteKey) {
Copy link
Owner

Choose a reason for hiding this comment

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

The name of the function is missleading.. it only returns certain attachments, won't it?

Copy link
Author

Choose a reason for hiding this comment

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

Well it get attachment paths for 3 type of attachments:

  • Existing attachments
  • Existing attachments that are in use
  • Existing attachments that aren't in use

What name should I put it? How about getAttachmentsPath?

Copy link
Owner

Choose a reason for hiding this comment

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

hm.. don't know.. if you stay with the name, can you add javadoc explaining what it returns?

browser/main/lib/dataApi/attachmentManagement.js Outdated Show resolved Hide resolved
@@ -57,8 +91,61 @@ class StoragesTab extends React.Component {
e.preventDefault()
}

removeAllAttachments (attachments) {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this method should also be in the attachmentManagement-file..?

Copy link
Owner

Choose a reason for hiding this comment

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

And a test for it is required

Copy link
Author

Choose a reason for hiding this comment

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

If I'm gonna put it in attachmentManagement, maybe I should change the getAttachments function return types to compatible with this. Well I plan to do that anyway, let's do it.

@@ -25,6 +27,20 @@ function browseFolder () {
})
}

function humanFileSize (bytes) {
Copy link
Author

Choose a reason for hiding this comment

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

Note to self: Move this into util file

@ZeroX-DG
Copy link
Author

@ehhc I have re-organized the code and added some comments for return types + added test. Tell me if I need to change anything else.

@ehhc
Copy link
Owner

ehhc commented Aug 28, 2019

I haven't tested your changes, but at first glance they look good

@ehhc ehhc merged commit a9b6598 into ehhc:option_for_attachment_delete Aug 28, 2019
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.

2 participants