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: add upload progress modal #1113

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

KristinAoki
Copy link
Member

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

The new Video Uploads modal will make it clear to users that closing the Videos page will cancel the upload and that refreshing the page will cancel the upload. Additionally, the modal includes a progress bar so that user can see approximately how far along the upload is.

Testing instructions

  1. Open the videos page
  2. Click "Add video"
  3. Select multiple videos to upload
  4. Upload in-progress modal should appear
  5. Confirm that clicking outside of the modal is disabled
  6. Each of the videos that were selected should be listed
  7. Confirm that the progress bars switch to failed
  8. Modal closes automatically once all videos are processed
  9. Should show upload error message for all failed uploads
  10. Select another set of videos to upload
  11. Upload in-progress should appear
  12. Before videos finish, click "Cancel uploads"
  13. Should show upload error message for failed uploads and cancelled uploads

@KristinAoki KristinAoki requested a review from a team as a code owner June 18, 2024 15:12
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 98.11321% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.63%. Comparing base (db1250e) to head (199f744).
Report is 3 commits behind head on master.

Files Patch % Lines
src/files-and-videos/videos-page/data/thunks.js 96.96% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@KristinAoki KristinAoki force-pushed the KristinAoki/add-upload-progress-modal branch from 69d4b0a to a8ec4a8 Compare June 18, 2024 19:05
Copy link
Member

@jesperhodge jesperhodge left a 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>
);
})}
Copy link
Member

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()}
Copy link
Member

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.

Copy link
Member

@jesperhodge jesperhodge left a 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

jesperhodge
jesperhodge previously approved these changes Jun 24, 2024
@KristinAoki KristinAoki merged commit 8ef804b into master Jun 24, 2024
6 checks passed
@KristinAoki KristinAoki deleted the KristinAoki/add-upload-progress-modal branch June 24, 2024 14:53
KristinAoki added a commit that referenced this pull request Jun 24, 2024
KristinAoki added a commit that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants