-
Notifications
You must be signed in to change notification settings - Fork 87
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: add upload progress modal #1113
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1113 +/- ##
==========================================
+ Coverage 92.58% 92.63% +0.04%
==========================================
Files 708 713 +5
Lines 12614 12749 +135
Branches 2770 2780 +10
==========================================
+ Hits 11679 11810 +131
- Misses 900 903 +3
- Partials 35 36 +1 ☔ View full report in Codecov by Sentry. |
69d4b0a
to
a8ec4a8
Compare
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.
Everything looks great, just some minor comments. I still need to test this though.
{getIcon()} | ||
</Stack> | ||
); | ||
})} |
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.
Instead of running a map data function inside of JSX, why not make the whole thing line 72 to 104 its own component? That will be clean, easily testable, and more readable and maintainable.
<ProgressBar now={video.uploadPercentage} variant="info" /> | ||
)} | ||
</div> | ||
{getIcon()} |
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.
This looks to me like an instance of the known renderThing()
antipattern. Calling a getIcon()
function just looks like an alternative way to a React component. How about instead of having a getIcon()
function having a RequestStatusIcon
component that does exactly the same? That's much more the React way.
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.
Code looks great, just need to test somehow
This reverts commit 8ef804b.
Description
The current implementation of video uploads notifies the user that there are uploads occurring but does not let the user cancel uploads not show the progress for individual uploads. This leads the user is suspense regarding which videos have completed the uploads. This PR add a modal the modal appears when the upload begins and disappears when upload is complete and progress bars as a visual representation to give the user a sense of how far along the upload is. This matches the upload flow of the legacy page with a new UI. This change impacts the Course Author.
Supporting information
JIRA Ticket: TNL-11629
Testing instructions