[UEPR-518] Implement in-editor Manual Save Project Thumbnail logic#471
Conversation
kbangelov
left a comment
kbangelov
left a comment
There was a problem hiding this comment.
I have not changed delete confirmation prompt to use our new element since it has some visual differences and such a change has not been communicated with the Scratch team yet
kbangelov
changed the title
KManolov3
left a comment
KManolov3
left a comment
There was a problem hiding this comment.
Good job! Overall, comments are minor, but I do wonder if the logic in confirmation-prompt can be simplified.
| border-radius: $form-radius; | ||
| } | ||
|
|
||
| [dir="ltr"] .stage-size-toggle-group { |
There was a problem hiding this comment.
Aren't those still used?
There was a problem hiding this comment.
The design in the task description doesn't have that extra margin which would make the gaps uneven. Tereza agreed on 4px (.25 rem) for both gaps
There was a problem hiding this comment.
Even so, I believe they're still being referred in the code
There was a problem hiding this comment.
That was only needed previously when it was considered the leftmost and only element in that group. Now I add the gap from the parent component, so I removed the div entirely.
There was a problem hiding this comment.
Pull request overview
Implements the UEPR-518 UX for manually saving a project thumbnail from within the editor (instead of the project page), including a confirmation prompt and new alert states/styling.
Changes:
- Move “Set Thumbnail” control into the in-editor Stage header and gate it by editor/project ownership state.
- Add a reusable
ConfirmationPromptmodal component with directional arrow assets and styling. - Add new thumbnail-related alerts (setting/success/error) and introduce a “blue” alert style.
Reviewed changes
Copilot reviewed 9 out of 15 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/lib/assets/icon--error.svg | Adds an error icon asset used by the new thumbnail failure alert. |
| packages/scratch-gui/src/lib/alerts/index.jsx | Adds new thumbnail alerts and introduces AlertLevels.BLUE. |
| packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx | Threads editor/ownership props into StageHeader. |
| packages/scratch-gui/src/components/stage-header/stage-header.jsx | Adds thumbnail button + confirmation flow and dispatches thumbnail alerts. |
| packages/scratch-gui/src/components/stage-header/stage-header.css | Adjusts layout spacing for the stage size row. |
| packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg | Adds thumbnail button icon. |
| packages/scratch-gui/src/components/gui/gui.jsx | Passes thumbnail/ownership/editor props into StageWrapper for editor mode. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg | Adds confirmation prompt arrow asset (up). |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg | Adds confirmation prompt arrow asset (right). |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg | Adds confirmation prompt arrow asset (left). |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg | Adds confirmation prompt arrow asset (down). |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx | Introduces new confirmation prompt modal + positioning logic. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css | Styles the confirmation prompt modal and buttons. |
| packages/scratch-gui/src/components/button/button.jsx | Adds componentRef prop to expose the underlying button element ref. |
| packages/scratch-gui/src/components/alerts/alert.css | Adds “blue” alert styling and adjusts alert box sizing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ConfirmationPrompt | ||
| isOpen={isThumbnailPromptOpen} | ||
| title={messages.setThumbnail} | ||
| message={<FormattedMessage {...messages.setThumbnailMessage} />} |
There was a problem hiding this comment.
The props being passed seem inconsistent here - strings in one place and FormattedMessage for the other props. I wonder if all strings makes more sense here?
| width={336} | ||
| title={intl.formatMessage(messages.thumbnailTooltipTitle)} | ||
| body={ | ||
| <FormattedMessage |
There was a problem hiding this comment.
I think I need to pass the node here because I don't see how else I'll be able to bold the text just inside the Tooltip component.
There was a problem hiding this comment.
You can consider intl.formatMessage as well
| const modalWidth = 200; | ||
| const spaceForArrow = 16; | ||
| const arrowOffsetFromEnd = 7; | ||
| const arrowLongSide = 29; | ||
| const arrowShortSide = 13; |
There was a problem hiding this comment.
If we truly want to make this component generic, we should make these configurable.
| {/* To remove - new feature awareness tooltip */} | ||
| <Tooltip | ||
| isOpen={isThumbnailTooltipOpen} | ||
| onRequestOpen={onOpenTooltip} | ||
| onRequestClose={onCloseTooltip} | ||
| targetRef={thumbnailButtonRef} | ||
| primaryPosition="left" | ||
| secondaryPosition="down" | ||
| width={336} | ||
| title={intl.formatMessage(messages.thumbnailTooltipTitle)} | ||
| body={ |
There was a problem hiding this comment.
The tooltip display is generally a part of another task - https://scratchfoundation.atlassian.net/browse/UEPR-522, so we don't need to handle it here. There are also more specific requirements that we need to implement, but I'd suggest leaving this as is for now and then finishing/updating the callout work as part of the other task.
| primaryPosition="down" | ||
| secondaryPosition="left" |
There was a problem hiding this comment.
Maybe rename these to side and align? I think it'd be easier for readers to understand which is which.
side - On which side of the anchor element should the prompt be displayed (top, bottom, left, right)
align - Where should the arrow be placed (left, center, right)
If we go with this approach, we'd also have to flip the logic for left and right for the secondary position. Right now, left indicates that most of the content is displayed on the left side of the button, but the arrow is displayed on the right side of the modal itself, which was not what I expected seeing this configuration.
There was a problem hiding this comment.
I can also see arrowPosition as an alternative naming for the align prop
There was a problem hiding this comment.
since secondaryPosition is more about where the entire modal is placed (instead of centrally) I'll rename it to align
| switch (primaryPosition) { | ||
| case 'up': | ||
| top = buttonRect.top - modalHeight - spaceForArrow; | ||
| break; | ||
| case 'down': | ||
| top = buttonRect.bottom + spaceForArrow; | ||
| break; | ||
| case 'left': | ||
| left = buttonRect.left - popupWidth - spaceForArrow; | ||
| break; | ||
| case 'right': | ||
| left = buttonRect.right + spaceForArrow; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Let's add these values to an enum, instead of hardcoding them
| const arrowLongSide = 29; | ||
| const arrowShortSide = 13; | ||
|
|
||
| const ConfirmationPrompt = ({ |
There was a problem hiding this comment.
Can we:
- pass the positioning params (width, arrow sizes, ..) as a configuration object to the ConfirmationPrompt
- reuse this component for the delete confirmation prompt, since it already contains a less complex version of the logic used here
There was a problem hiding this comment.
What I find slightly confusing about the current implementation is the following structure:
PopupWithArrowis a generic component used byConfirmationPrompt,DeleteConfirmationPrompt, andFeatureCallout.ConfirmationPromptis also a generic component, but it's currently only used for the thumbnail confirmation prompt and not for the delete confirmation prompt.
I think what we actually need is a clearer separation of responsibilities:
SetTooltipConfirmationPromptshould useConfirmationPromptand pass in specific content such astitle(if needed),description,className,layout, etc.DeleteConfirmationPromptshould also useConfirmationPromptand provide its own specific content.ConfirmationPromptshould usePopupWithArrow, which handles alignment and positioning.FeatureCalloutshould usePopupWithArrowfor alignment and positioning.PopupWithArrowshould useReactModalto leverage its built-in behavior for handling outside clicks, escape key presses, etc.
This structure would simplify the overall design and ensure that each component has a clear, single responsibility. We may still need to refine the naming to better reflect each component's purpose, but this is the general direction I think we should take.
| primaryPosition="down" | ||
| secondaryPosition="left" |
There was a problem hiding this comment.
I can also see arrowPosition as an alternative naming for the align prop
There was a problem hiding this comment.
Pull request overview
Implements “manual save project thumbnail” in the editor by moving the thumbnail action into the stage header UI and adding supporting UX (confirmation prompt, alerts, and new tooltip/popup positioning utilities).
Changes:
- Add editor-stage “Set Thumbnail” button with confirmation prompt + (temporary) feature-awareness tooltip.
- Add new alert types for thumbnail update progress/success/failure, including new styling/assets.
- Introduce shared popup positioning helper and new UI components (Tooltip, ConfirmationPrompt) to support the new interactions.
Reviewed changes
Copilot reviewed 18 out of 28 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/reducers/project-state.js | Adds getIsProjectLoadedWithId selector used to gate thumbnail UI behavior. |
| packages/scratch-gui/src/lib/store-project-thumbnail.js | Refactors thumbnail snapshot/save flow to async/await and promise-based behavior. |
| packages/scratch-gui/src/lib/assets/icon--error.svg | Adds error icon asset for new thumbnail-failure alert. |
| packages/scratch-gui/src/lib/alerts/index.jsx | Adds new thumbnail-related alerts and a new INFO_BLUE alert level. |
| packages/scratch-gui/src/hooks/calculatePopupPosition.js | Adds reusable popup/arrow positioning utility for tooltip/prompt UI. |
| packages/scratch-gui/src/css/colors.css | Adds new alert-related color tokens. |
| packages/scratch-gui/src/containers/stage-header.jsx | Wires redux state + alert dispatchers into the stage header. |
| packages/scratch-gui/src/components/tooltip/tooltip.jsx | Adds new Tooltip component used for feature-awareness messaging. |
| packages/scratch-gui/src/components/tooltip/tooltip.css | Adds styling for the new Tooltip component. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-up.svg | Adds tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-right.svg | Adds tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-left.svg | Adds tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-down.svg | Adds tooltip arrow asset. |
| packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx | Passes userOwnsProject down to stage header for gating thumbnail UI. |
| packages/scratch-gui/src/components/stage-header/stage-header.jsx | Implements new “Set Thumbnail” button, confirmation prompt, tooltip, and alert-driven async flow. |
| packages/scratch-gui/src/components/stage-header/stage-header.css | Updates layout and adds highlight styling for the new button/tooltip state. |
| packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg | Adds new thumbnail icon for the stage header button. |
| packages/scratch-gui/src/components/spinner/spinner.jsx | Adds a default prop intended for spinner color customization. |
| packages/scratch-gui/src/components/spinner/spinner.css | Adds styling for the new info-blue spinner level. |
| packages/scratch-gui/src/components/gui/gui.jsx | Moves thumbnail props wiring to StageWrapper (instead of earlier location). |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg | Adds confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg | Adds confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg | Adds confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg | Adds confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx | Adds new ConfirmationPrompt component used to confirm thumbnail set action. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css | Adds styling for the new ConfirmationPrompt component. |
| packages/scratch-gui/src/components/button/button.jsx | Adds componentRef support for positioning tooltip/prompt relative to the button. |
| packages/scratch-gui/src/components/alerts/alert.css | Adds new info-blue alert styling and updates warn styling to use shared tokens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.error('Project thumbnail save error', e); | ||
| // This is intentionally fire/forget because a failure | ||
| // to save the thumbnail is not vitally important to the user. | ||
| throw e; |
| await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => { | ||
| onUpdateProjectThumbnail( | ||
| projectId, | ||
| dataURItoBlob(dataURI), | ||
| resolve, | ||
| reject | ||
| ); | ||
| })); |
| export const getProjectThumbnail = (vm, callback) => new Promise((resolve, reject) => { | ||
| vm.postIOData('video', {forceTransparentPreview: true}); | ||
| vm.renderer.requestSnapshot(dataURI => { | ||
| vm.postIOData('video', {forceTransparentPreview: false}); | ||
| callback(dataURI); | ||
| const result = callback(dataURI); | ||
| result | ||
| .then(() => { | ||
| resolve(); | ||
| }) | ||
| .catch(e => { | ||
| reject(e instanceof Error ? e : new Error(String(e))); | ||
| }); |
| } | ||
|
|
||
| .button-row button:focus { | ||
| outline: auto; |
There was a problem hiding this comment.
I wonder if that's a sign not to use all:unset but rather change what actually needs to be changed? My concern is that we may unset some default button styles that we haven't though of testing. I don't have a strong preference though, that's just a thought.
| }); | ||
| try { | ||
| await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => { | ||
| onUpdateProjectThumbnail( |
There was a problem hiding this comment.
Hmm, onUpdateProjectThumbnail already accepts onSuccess, onError. Can we not pass the callbacks directly? Why do we need to await the promises?
| <FormattedMessage | ||
| {...messages.thumbnailTooltipBody} | ||
| values={{ | ||
| boldText: <b>{intl.formatMessage(messages.setThumbnail)}</b> |
There was a problem hiding this comment.
Instead of using formatMessage inside a FormattedMessage, you can do something like what I've done in this PR. Basically that way you can simply add <b>...</b> in the message definition and that would bold it.
There was a problem hiding this comment.
Can we address this? It generally seems like a bad idea to use formatMessage inside a FormattedMessage.
| }, [isOpen, updatePosition]); | ||
|
|
||
| // Click outside to close | ||
| useEffect(() => { |
There was a problem hiding this comment.
+1, most of these useEffects will become redundant if we switch to a Modal.
| } = {...defaultConfig, ...layoutConfig}; | ||
|
|
||
| const memoizedLayoutConfig = React.useMemo(() => ({ | ||
| popupWidth: modalWidth, |
There was a problem hiding this comment.
nitpick: Can we use either modalWidth or popupWidth everywhere for consistency?
| const arrowLongSide = 29; | ||
| const arrowShortSide = 13; | ||
|
|
||
| const ConfirmationPrompt = ({ |
There was a problem hiding this comment.
What I find slightly confusing about the current implementation is the following structure:
PopupWithArrowis a generic component used byConfirmationPrompt,DeleteConfirmationPrompt, andFeatureCallout.ConfirmationPromptis also a generic component, but it's currently only used for the thumbnail confirmation prompt and not for the delete confirmation prompt.
I think what we actually need is a clearer separation of responsibilities:
SetTooltipConfirmationPromptshould useConfirmationPromptand pass in specific content such astitle(if needed),description,className,layout, etc.DeleteConfirmationPromptshould also useConfirmationPromptand provide its own specific content.ConfirmationPromptshould usePopupWithArrow, which handles alignment and positioning.FeatureCalloutshould usePopupWithArrowfor alignment and positioning.PopupWithArrowshould useReactModalto leverage its built-in behavior for handling outside clicks, escape key presses, etc.
This structure would simplify the overall design and ensure that each component has a clear, single responsibility. We may still need to refine the naming to better reflect each component's purpose, but this is the general direction I think we should take.
| arrowRightIcon | ||
| }; | ||
|
|
||
| const BUTTON_ORDER = { |
There was a problem hiding this comment.
Ideally, I'd generalize the props to:
`mainButtonIcon`
`mainButtonStyles`
`mainButtonLabel`
`mainButtonAction`
`secondaryButtonIcon`
`secondaryButtonStyles`
`secondaryButtonLabel`
`secondaryButtonAction`
But I'm not insisting us do that here, as the confirm/cancel separation fits the current cases
| message, | ||
| confirmLabel, | ||
| cancelLabel, | ||
| confirmIcon, |
There was a problem hiding this comment.
Can we group the related props in a confirmButtonConfig, canceButtonConfig objects?
There was a problem hiding this comment.
Or alternatively, we can pass primaryButtonContent/secondaryButtonContent - a React node, which can either be the text if that's all we want to display, or text + icon. Both seem fine to me, whatever you feel is better.
| isOpen | ||
| onRequestClose={onCancel} | ||
| title={intl.formatMessage(messages.confirmDeletionHeading)} | ||
| message={<FormattedMessage {...getMessage(entityType)} />} |
There was a problem hiding this comment.
It looks strange to use FormattedMessage for the message and intl.formatMessage for the title. Any reason to have it differ?
There was a problem hiding this comment.
Yes, in ConfirmationPrompt I use css styles that I apply to spans and FormattedMessage renders one. And title is just passed purely for accessibility.
| margin: auto; | ||
| } | ||
|
|
||
| .button-row button.ok-button { |
There was a problem hiding this comment.
Feelsgood to remove this code
| className={styles.buttonIcon} | ||
| src={confirmIcon} | ||
| aria-hidden="true" | ||
| alt="" |
There was a problem hiding this comment.
nitpick: Maybe we shouldn't leave the alt text empty, since we are also working on making the editor more accessible? Same for the cancel button?
There was a problem hiding this comment.
Since the images are not supposed to be announced, I'll just remove the alt props alltogether.
| try { | ||
| storeProjectThumbnail(vm, dataURI => { | ||
| onUpdateProjectThumbnail(projectId, dataURItoBlob(dataURI)); | ||
| onUpdateProjectThumbnail( | ||
| projectId, | ||
| dataURItoBlob(dataURI), | ||
| () => { | ||
| onShowThumbnailSuccess(); | ||
| setIsUpdatingThumbnail(false); | ||
| }, | ||
| () => { | ||
| onShowThumbnailError(); | ||
| setIsUpdatingThumbnail(false); | ||
| } | ||
| ); | ||
| }); | ||
| }, | ||
| 3000 | ||
| ), | ||
| [projectId, onUpdateProjectThumbnail] | ||
| } catch (e) { | ||
| onShowThumbnailError(); |
There was a problem hiding this comment.
Would we ever get to the catch of this try/catch block? If the thumbnail update fails, it'll call the onError callback, but shouldn't fire an error that we can can here? The storeProjectThumbnail has its own try/catch block, which swallows the error, so it wouldn't bubble up to here?
adzhindzhi
left a comment
adzhindzhi
left a comment
There was a problem hiding this comment.
I left a few small comments, but overall it looks good to me! 🙌 I also noticed that the FeatureCalloutPopover is not positioned correctly on initial render, but I think that can be addressed when we start working on the feature callouts.
| <FormattedMessage | ||
| {...messages.thumbnailTooltipBody} | ||
| values={{ | ||
| boldText: <b>{intl.formatMessage(messages.setThumbnail)}</b> |
There was a problem hiding this comment.
Can we address this? It generally seems like a bad idea to use formatMessage inside a FormattedMessage.
| const getIsProjectLoadedWithId = loadingState => ( | ||
| getIsShowingWithId(loadingState) || | ||
| getIsUpdating(loadingState) | ||
| ); |
There was a problem hiding this comment.
Usually, for a loaded project we're using getIsShowingWithId, is there a particular reason why you want to consider an updating project as "loaded"? Wouldn't this also mark new projects that are currently creating as loaded? We're using flag only for whether or not to show the Set Thumbnail button, right? Would it make a difference if we show the button if the project isn't entirely loaded?
There was a problem hiding this comment.
As I expected, it causes the icon to reload when we, for example, update the project. I'll replace it with getIsShowingWithId, since it seems to happen fast. Although I don't know if there should be a link between saving the project and saving the thumbnail.
There was a problem hiding this comment.
Well, to be fair, in that case I don't really see a valid reason to check whether the project is loading/saved at all. Is there a particular reason why this check was added? If not, maybe we can remove it altogether and just always display the button if the feature flag is on and the user owns the project?
| const onPopupMount = useCallback(el => { | ||
| if (!el || !isOpen) return; | ||
| modalRef.current = el; | ||
| updatePosition(); | ||
| }, [isOpen, updatePosition]); |
There was a problem hiding this comment.
nitpick: You can probably pass the modalRef to the contentRef directly and just update the position in the useEffect when isOpen changes
There was a problem hiding this comment.
I prefer this, because it updates position when the element has already mounted.
There was a problem hiding this comment.
I think we should get rid of the use effect if we can. If the only concern is running updatePosition one extra time on mount, then it shouldn't be much of an issue.
There was a problem hiding this comment.
Pull request overview
Implements an in-editor “Set Thumbnail” flow by moving the control into the editor’s stage header and adding confirmation + alert UX around the update action.
Changes:
- Moves manual thumbnail update into the editor stage header with a confirmation prompt and update-state alerts (in-progress / success / error).
- Introduces reusable arrow-positioned popup infrastructure (
ModalWithArrow,ConfirmationPrompt,FeatureCalloutPopover) and a shared positioning helper. - Adds new alert/spinner styling and icon assets to support the updated UX.
Reviewed changes
Copilot reviewed 19 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/lib/calculatePopupPosition.js | Adds shared popup + arrow positioning helper with side/align enums. |
| packages/scratch-gui/src/lib/assets/icon--error.svg | Adds error icon asset for thumbnail failure alert. |
| packages/scratch-gui/src/lib/alerts/index.jsx | Adds new thumbnail-related alerts and an info-blue alert level. |
| packages/scratch-gui/src/css/colors.css | Adds alert overlay/orange color tokens used by new alert styles. |
| packages/scratch-gui/src/containers/stage-header.jsx | Wires stage header to project loaded state + dispatches new thumbnail alerts. |
| packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx | Plumbs userOwnsProject down to StageHeader. |
| packages/scratch-gui/src/components/stage-header/stage-header.jsx | Implements in-editor thumbnail button, confirmation prompt, and alert-driven updating state. |
| packages/scratch-gui/src/components/stage-header/stage-header.css | Updates layout and adds highlighted styling for the new thumbnail button placement. |
| packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg | Adds thumbnail icon for the new stage-header button. |
| packages/scratch-gui/src/components/spinner/spinner.css | Adds info-blue spinner styling for the “setting thumbnail” alert. |
| packages/scratch-gui/src/components/modal-with-arrow/modal-with-arrow.jsx | Adds a reusable modal wrapper that positions itself + an arrow relative to a target element. |
| packages/scratch-gui/src/components/modal-with-arrow/modal-with-arrow.css | Adds styling for the fixed-position arrow modal. |
| packages/scratch-gui/src/components/gui/gui.jsx | Adjusts prop wiring so StageWrapper receives manuallySaveThumbnails, userOwnsProject, and onUpdateProjectThumbnail. |
| packages/scratch-gui/src/components/feature-callout-popover/icon--arrow-up.svg | Adds arrow asset for callout popover. |
| packages/scratch-gui/src/components/feature-callout-popover/icon--arrow-right.svg | Adds arrow asset for callout popover. |
| packages/scratch-gui/src/components/feature-callout-popover/icon--arrow-left.svg | Adds arrow asset for callout popover. |
| packages/scratch-gui/src/components/feature-callout-popover/icon--arrow-down.svg | Adds arrow asset for callout popover. |
| packages/scratch-gui/src/components/feature-callout-popover/feature-callout-popover.jsx | Adds feature callout popover component built on ModalWithArrow. |
| packages/scratch-gui/src/components/feature-callout-popover/feature-callout-popover.css | Adds styling for the feature callout UI. |
| packages/scratch-gui/src/components/delete-confirmation-prompt/delete-confirmation-prompt.jsx | Refactors delete prompt to use the new ConfirmationPrompt/arrow modal infrastructure. |
| packages/scratch-gui/src/components/delete-confirmation-prompt/delete-confirmation-prompt.css | Updates delete prompt styles to match new prompt component structure. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg | Adds arrow asset for confirmation prompt. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg | Adds arrow asset for confirmation prompt. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg | Adds arrow asset for confirmation prompt. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg | Adds arrow asset for confirmation prompt. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx | Adds a reusable confirmation prompt component built on ModalWithArrow. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css | Adds styling for the confirmation prompt UI. |
| packages/scratch-gui/src/components/button/button.jsx | Adds componentRef support so callers can anchor popups to button DOM nodes. |
| packages/scratch-gui/src/components/alerts/alert.css | Updates alert styling (warn colors, new info-blue alert styling, box-sizing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onUpdateProjectThumbnail( | ||
| projectId, | ||
| dataURItoBlob(dataURI), | ||
| () => { | ||
| onShowThumbnailSuccess(); | ||
| setIsUpdatingThumbnail(false); | ||
| }, | ||
| () => { | ||
| onShowThumbnailError(); | ||
| setIsUpdatingThumbnail(false); | ||
| } | ||
| ); |
There was a problem hiding this comment.
onUpdateProjectThumbnail is being called with success/error callbacks, but the configured storage API in this repo (GUIStorage.saveProjectThumbnail in gui-config.ts) only takes (projectId, thumbnail) and existing call sites pass 2 args. If the implementation ignores extra args (typical JS), these callbacks will never run, leaving isUpdatingThumbnail stuck true (button disabled) and never showing success/error alerts. Consider updating onUpdateProjectThumbnail to return a Promise (and update typings/usages) so this component can .then/.catch/.finally, or otherwise ensure setIsUpdatingThumbnail(false) and the correct alert happens even when callbacks aren’t supported.
| onUpdateProjectThumbnail( | |
| projectId, | |
| dataURItoBlob(dataURI), | |
| () => { | |
| onShowThumbnailSuccess(); | |
| setIsUpdatingThumbnail(false); | |
| }, | |
| () => { | |
| onShowThumbnailError(); | |
| setIsUpdatingThumbnail(false); | |
| } | |
| ); | |
| const handleSuccess = () => { | |
| onShowThumbnailSuccess(); | |
| setIsUpdatingThumbnail(false); | |
| }; | |
| const handleError = () => { | |
| onShowThumbnailError(); | |
| setIsUpdatingThumbnail(false); | |
| }; | |
| const maybePromise = onUpdateProjectThumbnail( | |
| projectId, | |
| dataURItoBlob(dataURI), | |
| handleSuccess, | |
| handleError | |
| ); | |
| if (maybePromise && typeof maybePromise.then === 'function') { | |
| maybePromise.then(handleSuccess).catch(handleError); | |
| } else if (onUpdateProjectThumbnail.length < 3) { | |
| // If callbacks are not supported and no Promise is returned, | |
| // ensure the updating state is cleared so the UI is not stuck. | |
| setIsUpdatingThumbnail(false); | |
| } |
| confirmLabel: PropTypes.node, | ||
| cancelLabel: PropTypes.node, |
There was a problem hiding this comment.
confirmLabel/cancelLabel are listed in ConfirmationPrompt.propTypes, but the component doesn’t use them (it uses confirmButtonConfig.label / cancelButtonConfig.label). Either wire these props into the render logic or remove them to avoid a misleading API surface.
| confirmLabel: PropTypes.node, | |
| cancelLabel: PropTypes.node, |
KManolov3
left a comment
KManolov3
left a comment
There was a problem hiding this comment.
Looks good to me! 🙌 Some very minor comments left
KManolov3
changed the base branch from
develop
to
release/manual-thumbnail-update-in-editor
KManolov3
merged commit 8990f38
into
scratchfoundation:release/manual-thumbnail-update-in-editor
Resolves
https://scratchfoundation.atlassian.net/browse/UEPR-518
Proposed Changes
Moves the "update thumbnail" button from project page to project editor and adds:
Design here : https://www.figma.com/design/nQK2PbAYG7pCmK4lYOTvHZ/Save-Thumbnail?node-id=370-1473&p=f&m=dev
To Do