-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
base: main
Are you sure you want to change the base?
Tus upload 5423 #6351
Conversation
…loading the files using the tus-uploady react library
✅ Deploy Preview for plone-components canceled.
|
@@ -51,6 +51,10 @@ const BreadcrumbsComponent = ({ pathname }) => { | |||
} | |||
}, [dispatch, pathname]); | |||
|
|||
if (pathname.endsWith('/uploads')) { |
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.
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
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.
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.
@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: |
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'm not sure of this, will need to speak to @djay and @JeffersonBledsoe about this.
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. |
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. |
@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. |
Fixes #5423.
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.