-
Notifications
You must be signed in to change notification settings - Fork 7
Adding Media Library to labs #172
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
base: main
Are you sure you want to change the base?
Conversation
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.
Where does the media-library-full.iife.js get built from? from the DA project by any chance? Wondering if we can not have it minified unless it's ridiculously large and causing perf issues?
tools/media-library/scripts.js
Outdated
} | ||
|
||
async function fetchSitemap(sitemapURL) { | ||
const fetchUrl = `https://little-forest-58aa.david8603.workers.dev/?url=${encodeURIComponent(sitemapURL)}`; |
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.
shouldn't we run this in our own POC CF space?
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 is the cors proxy that is being used in image audit . The web component will take a cors proxy as property so that host systems can use their own. The web component code it self uses the cors proxy deployed into poc space by default. Since this will be part of labs i thought it is better to use the existing cors proxy. Please let me know if that is not the case
tools/media-library/scripts.js
Outdated
clearButton.style.display = display; | ||
} | ||
|
||
async function initializeMediaLibraryWithRetry(mediaLibrary, storageType) { |
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.
seems like we can combine this function with initializeMediaLibrary to be one function
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.
cleaned up and addressed
tools/media-library/scripts.js
Outdated
|
||
setupStorageManager(mediaLibrary, siteKey); | ||
|
||
await new Promise((resolve) => { |
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.
not a big fan of timeouts. Instead of a timeout, can we listen for a custom event to know when the media library has loaded and then based on that custom event we can do something when triggered?
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.
addressed in the new code. relying on the component ready event
https://github.com/kmurugulla/media-library-wc is the repository for web component . Added the un minified version of the web component script to deps and ignoring it from jslint |
@kmurugulla thanks for the fixes. I'd say let's wait to merge this for now as discussed. |
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Fix #
Test URLs: