feat: fetch dynamic assets and pass them on to GUI#10008
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to fetch dynamic assets from the scratchr2 backend and pass them to the Scratch GUI editor. The implementation fetches assets from the /mediagallery/dynamic-assets/ endpoint on component mount and stores them in the component state.
Changes:
- Added
fetchDynamicAssetsmethod to retrieve dynamic assets from the backend API - Initialized
dynamicAssetsstate property and passed it as a prop toIntlGUIWithProjectHandler - Configured the fetch to occur on component mount via
componentDidMount
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| adminPanelOpen: adminPanelOpen || false, | ||
| clientFaved: false, | ||
| clientLoved: false, | ||
| dynamicAssets: {}, |
There was a problem hiding this comment.
The initial state is set to an empty object, but there's no handling for the loading state. Consider adding a loading flag or checking if dynamicAssets is empty in the GUI to distinguish between 'not yet loaded' and 'loaded with no assets'.
| dynamicAssets: {}, | |
| dynamicAssets: {}, | |
| dynamicAssetsLoading: true, |
There was a problem hiding this comment.
I think there's a design question here about whether or not to show a spinner (or something) to indicate that it's loading. That said, this specific suggestion doesn't seem necessary even if we do end up wanting a loading indicator.
cwillisf
left a comment
cwillisf
left a comment
There was a problem hiding this comment.
I'd add at least console-level error reporting, but otherwise, this looks great!
| adminPanelOpen: adminPanelOpen || false, | ||
| clientFaved: false, | ||
| clientLoved: false, | ||
| dynamicAssets: {}, |
There was a problem hiding this comment.
I think there's a design question here about whether or not to show a spinner (or something) to indicate that it's loading. That said, this specific suggestion doesn't seem necessary even if we do end up wanting a loading indicator.
src/views/preview/project-view.jsx
Outdated
| uri: '/mediagallery/dynamic-assets/' | ||
| }, (err, body, res) => { | ||
| if (err || (res && res.statusCode >= 400)) { | ||
| // TODO: Should we show an error message? |
There was a problem hiding this comment.
I think it’s fine to not show an error to the user here. This happens “behind the scenes” when opening the editor, and I think it would confuse users more than it would actually help 😅 That said, I agree with Copilot that it makes sense to at least log the error.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4 participants
Resolves:
https://scratchfoundation.atlassian.net/browse/UEPR-467
Changes:
Fetch dynamic assets from scratchr2 and pass them on to the editor
Matching PRs:
scratchfoundation/scratch-editor#412
https://github.com/scratchfoundation/scratchr2/pull/6780