Skip to content
This repository was archived by the owner on Feb 14, 2025. It is now read-only.

feat: improve asset loading - #484

Merged
KristinAoki merged 10 commits into
mainfrom
KristinAoki/improve-asset-loading
Jun 17, 2024
Merged

KristinAoki merged 10 commits into
mainfrom
KristinAoki/improve-asset-loading

Conversation

@KristinAoki

@KristinAoki KristinAoki commented Jun 10, 2024

Copy link
Copy Markdown
Contributor

Description

Remove unnecessary API calls

This PR improves the load time of the text editor, problem editor, and video editor. Previously all data that was required for any editor type at initialization was loaded, even if it was not necessary for the editor in-use. This means the text editor was loading course videos even it would never be using that data. Now the initialization function checks the editor and only calls the required APIs.

Reducing the need for all assets at load time

The need for all course assets to be fetched during initialization was removed. Now only the first fifty (50) images for a course are loaded for the text and problem editors. Previously all course assets were needed to update and write /static/ URLs, but assets all have a specific key pattern so the replace and set functions for the URLs are able to use a pattern matcher rather then copying data from the asset object.

Adding Lazy Load for SelectImageModal

To improve load time, the number of image assets fetched at load time was reduced to fifty (50), the default page size. However a course can have more than fifty images, so lazy loading was added to the SelectImageModal. This makes it quicker to load the editor and makes it easier for the user to scroll through the images. In the case that there are only fifty images or less for a course or the images are being filter are less than or equal to fifty, the load more button will not appear.

Supporting Information

JIRA Ticket: TNL-10869

Lazy-Loading assets like images, transcripts, and handouts.
- This is for blocks that have asset galleries that display all course assets (html, problem)
- Should not impact loading of images that are directly referenced in a block already, nor should it impact the adding of static URLs in the source code view for html blocks

Testing

  1. Open each of the editors and check the logs to see which actions have been called
  • The text and problem editors should call fetchBlock, fetchUnit, and fetchImages
  • The video editor should call fetchBlock, fetchUnit, fetchVideos, fetchCourseDetailsView, fetchStudioView
  1. Add a new image to a html block
  2. Click the "HTML" button
  3. Confirm that all the images are relative URLS
  4. Confirm that all the relative URLS follow asset-v1:{org}+{number+{run}+type@asset+block@{key_safe_name}
  5. Click the "HTML" button
  6. Update one of the relative URLs for an image to /static/{key_safe_name} (aka the Studio URL)
  7. Click "Save"
  8. Confirm that the image is still viewable in the editor
  9. Click "Save"
  10. Confirm that all links and images in html block are viewable on the unit page
  11. Click "Edit"
  12. Confirm that block loads as expected
  13. Open a video block
  14. Click "Replace video"
  15. Confirm that list displays as expected
  16. Add a name to search by
  17. Confirm that the expect results are displayed

@rayzhou-bit rayzhou-bit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have one small change for you to look at. Overall, this looks great!

},
},
"isRaw": false,
"showRawEditor": false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this name change

setImages: (state, { payload }) => ({
...state,
images: { ...state.images, ...payload.images },
imageCount: payload.imageCount,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this add to the existing imageCount? I'm assuming imageCount should always reflect number of images

Suggested change
imageCount: payload.imageCount,
imageCount: state.imageCount + payload.imageCount,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The image count is the total number of assets in a course, not the number in the most recent fetch. The image count should be overwritten each time to make sure the most recent asset total on each fetch.

@rayzhou-bit
rayzhou-bit self-requested a review June 12, 2024 18:31

@rayzhou-bit rayzhou-bit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Note that problem editor calls api for advanced settings data which isn't mentioned in the description, but this is a necessary api call.

@KristinAoki

Copy link
Copy Markdown
Contributor Author

I did not mention that in the description because that API call is not part of the initialization call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants