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

feat(upload-extension): improve upload extension #2096

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shlroland
Copy link
Contributor

Description

Checklist

  • I have read the contributing document.
  • My code follows the code style of this project and pnpm fix completed successfully.
  • I have updated the documentation where necessary.
  • New code is unit tested and all current tests pass when running pnpm test.

Screenshots

@changeset-bot
Copy link

changeset-bot bot commented Jun 26, 2023

⚠️ No Changeset found

Latest commit: 59e02c3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@shlroland shlroland changed the title Fix/improve upload extension Feat/improve upload extension Jun 26, 2023
@github-actions github-actions bot added the test: unit ✅ A label for updating tests and test configuration. label Jun 26, 2023
@shlroland shlroland changed the title Feat/improve upload extension feat(upload-extension): improve upload extension Jun 26, 2023
Comment on lines +30 to +34
export interface PlaceholderPluginMeta {
added: Array<Omit<AddPlaceholderAction, 'type'>>;
removed: Array<Omit<RemovePlaceholderAction, 'type'>>;
updated: Array<Omit<UpdatPlaceholderAction, 'type'>>;
}
Copy link
Member

Choose a reason for hiding this comment

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

If PlaceholderPluginMeta is just an array of PlaceholderPluginAction, it seems both the typing could be simpler (e.g. no Omit is needed anymore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though the type property in PlaceholderPluginMeta doesn't have much use, keeping it directly can make the code look a bit simpler. I'll have a try.

@ocavue
Copy link
Member

ocavue commented Jun 26, 2023

Your change for setUploadPlaceholderAction looks good to me. I'm thinking maybe we can move it forward and add multiple files uploading feature to uploadFile, and maybe also relative places in the @remirror/extension-file. After all, setUploadPlaceholderAction is most like an internal API and most people won't use it directly.

@shlroland
Copy link
Contributor Author

Your change for setUploadPlaceholderAction looks good to me. I'm thinking maybe we can move it forward and add multiple files uploading feature to uploadFile, and maybe also relative places in the @remirror/extension-file. After all, setUploadPlaceholderAction is most like an internal API and most people won't use it directly.

I agree with your perspective. It seems like we need to separate the functionality into three methods: add remove and update, to make them directly available for users to use.
The current uploadFiles is still functional, treating each file individually for uploading. If I understand correctly, you're asking if we should consider it as an array of files in the file-upload to achieve the uploading functionality, is that right?

@ocavue
Copy link
Member

ocavue commented Jun 27, 2023

The current uploadFiles is still functional, treating each file individually for uploading. If I understand correctly, you're asking if we should consider it as an array of files in the file-upload to achieve the uploading functionality, is that right?

Current uploadFiles already support multiple files, by dispatching multiple individual transactions. With your change in this PR, we can achieve that within single transaction, which could bring better performance more or less.

I agree with your perspective. It seems like we need to separate the functionality into three methods: add remove and update, to make them directly available for users to use.

By using uploadFiles, the editor will trigger AddPlaceholderAction and RemovePlaceholderAction (when the uploading is done). Speaking of that, I would like to know in which scene people would want to use UpdatPlaceholderAction. We should write some kind of document, unit test, or a storybook story to show such scene.

@shlroland shlroland force-pushed the fix/improve_upload_extension branch from 1afcce1 to d62be92 Compare June 28, 2023 11:38
@shlroland
Copy link
Contributor Author

shlroland commented Jun 28, 2023

By using uploadFiles, the editor will trigger AddPlaceholderAction and RemovePlaceholderAction (when the uploading is done). Speaking of that, I would like to know in which scene people would want to use UpdatPlaceholderAction. We should write some kind of document, unit test, or a storybook story to show such scene.

I noticed your PR #2100. The modifications in this PR caused some of my current test cases to fail. So I think there is a tight coupling between the current upload-extension and file-extension, so I believe that the file-placeholder-plugin should exist as a plugin for file-extension rather than being a built-in extension.In this case, the file-placeholder-plugin can be used as a specific placeholder for the file-extension, while users can utilize the relevant methods from the decoration-extension when they need to use general placeholder functionality.

@shlroland
Copy link
Contributor Author

shlroland commented Jun 28, 2023

Speaking of that, I would like to know in which scene people would want to use UpdatPlaceholderAction.

In my project, the use case for the upload-placeholder is in scenarios where asynchronous requests are needed. A typical example is when I need to call a backend API to re-sign copied resource nodes, ensuring that the permissions of the resource nodes are not compromised.

@shlroland
Copy link
Contributor Author

It's worth noting that currently, the uploadImage in the image-extension is calling the placeholder method within the decoration-extension. When building rich text editor, the upload interfaces for images and files might often be the same, but they require adapting to two different upload logics, which can be confusing. Perhaps we should find a way to unify these two upload logics while keeping compatibility.

@shlroland shlroland force-pushed the fix/improve_upload_extension branch 3 times, most recently from 8919e06 to 7a280e2 Compare August 15, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test: unit ✅ A label for updating tests and test configuration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants