-
Notifications
You must be signed in to change notification settings - Fork 31
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
Better error management on assets upload #1311
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…ladev/errors-ondemand
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.
Just tested and works great 🔥
I just have a couple styling improvements I'd like to get made before we merge.
- Can we add some space between the first and second column? See the figma file for proper spacing.
- Can we replace the delete icon with the radix icon trash icon (
<TrashIcon />
) and give it a margin right of$2
? Also, the circular gray background behind the icon should only appear on hover. Can give it a slight fade in on hover (ietransition: 0.2s
).
Added a 500ms delay in the tooltips shown in assets tables
children: ( | ||
<Box> | ||
{sourceUrl && | ||
(sourceUrl.indexOf("https://livepeercdn.com") === 0 || |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization
<Box> | ||
{sourceUrl && | ||
(sourceUrl.indexOf("https://livepeercdn.com") === 0 || | ||
sourceUrl.indexOf("https://cdn.livepeer.com") === 0) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization
@adamsoffer I solved both issues. So whenever you want you can re-review and merge |
Hey @clacladev - I'm trying to test this on the preview url. Do you have a file that you know will fail processing so I test on my end? |
Nevermind, was able to just rename a file to .mp4 to test |
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.
Looks fantastic :)
What does this pull request do? Explain your changes. (required)
Add processing issues on asset list and on file upload dialog.
Specific updates (required)
Added error info to the Asset table item (red name, error icon, error badge with short info and more error details in the tooltips)
Added error info inside the File Upload dialog
Added the ability to Delete an asset which failed to process
Limited the files that can be uploaded to mp4
How did you test each of these updates (required)
Tested on light and dark theme.
Does this pull request close any open issues?
Fixes #1250
Screenshots (optional):
Error state for an asset entry:
Error details on asset tooltip:
Error info inside the File Upload:
Delete button for failed processing assets (loading and normal state):
File upload only allows mp4 files:
Checklist: