[go: up one dir, main page]

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

Tus upload 5423 #6351

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

Conversation

victorchrollo14
Copy link
Member

Fixes #5423.

  • Most of the stuff was done by @pretagov, the express-middlware and tus-uploady implementation for tus uploads. I just added some ui design with some modification of the tus-uploady implementation.
  • new libraries added for this are as follows
    "@rpldy/tus-uploady": "^1.8.3",
     "@rpldy/upload-drop-zone": "^1.8.3",
     "@rpldy/uploady": "^1.8.3",
     "body-parser": "^1.20.2",

  • How it looks like.
volto-tus-sep28.mp4

Few Issues with the current implementation.

  • Should we show some sort of a toast or a confirm modal when deleting an uploaded file.

  • The back button shows a abortall modal when uploads are in progress, if not it just redirects to the contents page, so when the user clicks on back button during upload he gets an option to abort all, but the tus-uploady doesn't have a pause option, so we can't pause the uploads, so by the time user clicks on abort all button, the uploads might be completed. (possible solutions: add a abort all button next to the upload and back button, which would just abort all the uploads and the back button would just serve as a back button )

  • when the upload speed is really fast like 100mb/s per second or something, there are too many re-renders because of which the abort button, delete button, abort all button doesn't work on a single click, you have click it multiple times. but this probably only occurs locally, when I reduce the network speed in the browser and the upload speed is normal this issue doesn't occur.

  • we get some string invalid error on file with invalid strings, what do we do about these kind of files, should we just not add them to the uploads at all or show some sort of an error message on the ui telling that the file name is invalid, so change the file name and upload.

20240928_11h26m25s_grim

  • all video file uploads seems to fail for some reason, it throws a 401 unauthorized error at one of the patch requests.
    image

Copy link
netlify bot commented Sep 28, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 88c7db9
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66fd2ad215cbdb0008b3d3ae

@@ -51,6 +51,10 @@ const BreadcrumbsComponent = ({ pathname }) => {
}
}, [dispatch, pathname]);

if (pathname.endsWith('/uploads')) {
Copy link
Member

Choose a reason for hiding this comment

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

there has to be a better way to avoid rendering certain components based on some route.
Try https://training.plone.org/effective-volto/addons/views.html#router-views
to add uploads to nonContentRoutes and see if this doesn't solve the content rendering

Copy link
Member Author

Choose a reason for hiding this comment

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

nope this didn't work. all the contents are rendered under app.jsx where the header, breadcrumbs and footer components are static and the content-area is dynamically rendered based on the route.
https://github.com/plone/volto/blob/main/packages/volto/src/components/theme/App/App.jsx, maybe we could move the logic the app.jsx file instead of having it seperately in header, footer and breadcrumbs file.

@ichim-david
Copy link
Member
ichim-david commented Sep 29, 2024

@plone/volto-team I have a feeling that this feature is arriving a bit too late to the scene with what we're trying to get to which is the removal of Semantic UI from the core, to use plone.contents instead of the contents we have right now and to slim the bundle.

I would rather have this feature as an add-on instead of in core given our future roadmap and after the implementation matures to see what we are willing to add to Volto core.

Please give your opinion as well on how you feel about this PLIP implementation and what do you think makes sense to have in core concerning TUS upload for contents.

I am adding it to the volto team meeting project and hopefully, we can have a discussion and get the opinion of the other members as well.

EDIT:
During the Salamina sprint we had a discussion about having a designation of core or recommended add-ons, I could see this add-on being a core add-on for Volto 18 and less websites since it's built using Semantic UI and it provides a useful feature for those that need it.
This makes it shippable now and directly useful for our current websites.
Along the way, it could be updated to a major release where it's based on plone.components or plone.contents or whatever makes sense at that time.
Again I am not against this work but I think it should live on as an add-on and not have it in core given our moving pieces and our future roadmap.

@victorchrollo14
Copy link
Member Author

@plone/volto-team I have a feeling that this feature is arriving a bit too late to the scene with what we're trying to get to which is the removal of Semantic UI from the core, to use plone.contents instead of the contents we have right now and to slim the bundle.

yeah I came across the plip to remove semantic ui after I made the PR. the plan seems to be to use @plone/components instead of the semantic ui stuff right, which seems to be using th react-spectrum library for the ui components.

I would rather have this feature as an add-on instead of in core given our future roadmap and after the implementation matures to see what we are willing to add to Volto core.

Please give your opinion as well on how you feel about this PLIP implementation and what do you think makes sense to have in core concerning TUS upload for contents.

I'm not sure of this, will need to speak to @djay and @JeffersonBledsoe about this.

EDIT: During the Salamina sprint we had a discussion about having a designation of core or recommended add-ons, I could see this add-on being a core add-on for Volto 18 and less websites since it's built using Semantic UI and it provides a useful feature for those that need it. This makes it shippable now and directly useful for our current websites. Along the way, it could be updated to a major release where it's based on plone.components or plone.contents or whatever makes sense at that time. Again I am not against this work but I think it should live on as an add-on and not have it in core given our moving pieces and our future roadmap.

Makes sense. since the new plone version would have removed semantic ui and we already have a tus implementation which uses semantic ui, we could use this as an addon for the current and older versions of volto. and maybe later implement it as a core part of volto in the newer versions.

@djay
Copy link
Member
djay commented Oct 1, 2024

This is one PR which is part of a bigger PLIP in #5423 so discussion of it needs to be had with regard to that PLIP rather than this PR in isolation. We should follow the PLIP process.
Note there is still another PR to come about changing the restapi to allow TUS api to be used in an out of the box Plone install without requiring a special shared temp dir.

@sneridagh
Copy link
Member

@ichim-david I agree to all your points. I would not make this a blocker for Volto 18 to happen. If it does not break anything, in the form of an optional core add-on, it can be added later to 18.1 as a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs discussion
Development

Successfully merging this pull request may close these issues.

PLIP: TUS reliable large uploads with simpler file upload UI in volto and better feedback on slow connections
4 participants