[UEPR-518] Implement in-editor Manual Save Project Thumbnail logic#471
Conversation
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
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.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css
Show resolved
Hide resolved
| 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
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css
Outdated
Show resolved
Hide resolved
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.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Show resolved
Hide resolved
packages/scratch-gui/src/components/stage-header/stage-header.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/stage-header/stage-header.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/stage-header/stage-header.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/stage-header/stage-header.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Outdated
Show resolved
Hide resolved
| <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 int.formatMessage as well
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css
Outdated
Show resolved
Hide resolved
| 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.
| const arrowLongSide = 29; | ||
| const arrowShortSide = 13; | ||
|
|
||
| const ConfirmationPrompt = ({ |
There was a problem hiding this comment.
Can we reuse this new ConfirmationPrompt for the DeleteConfirmationPrompt?
| {/* 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
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Show resolved
Hide resolved
| arrowShortSide, | ||
| arrowLongSide, | ||
| arrowOffsetFromEnd, | ||
| arrowOffsetFromBottom = 0 |
There was a problem hiding this comment.
Hmmm, I wonder if we can avoid any of these by simply passing arrowOffset? For example, what's the difference between spaceForArrow and arrowOffsetFromBottom? I think we simply need to how the arrowHeight and the offset from the anchor element?
Also, arrowOffsetFromEnd is a bit specific, I wonder if this should be something configurable at all. If anything, it should at least have a default value, because the cases of when we'd want to change that would be rare.
There was a problem hiding this comment.
II agree, let's add enough configuration for the cases we currently have, and if needed we can always expand the configuration later.
| arrowOffsetFromBottom = 0 | ||
| }) => { | ||
| const modalHeight = popupRef.current.getBoundingClientRect().height; | ||
| const arrowHeight = (primaryPosition === 'left' || primaryPosition === 'right') ? arrowLongSide : arrowShortSide; |
There was a problem hiding this comment.
Nitpick - maybe we could
const [arrowHeight, arrowWidth] = (primaryPosition === 'left' || primaryPosition === 'right') ? [arrowLongSide, arrowShortSide] : [arrowShortSide, arrowLongSide];
| 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
| primaryPosition="down" | ||
| secondaryPosition="left" |
There was a problem hiding this comment.
I can also see arrowPosition as an alternative naming for the align prop
| arrowShortSide, | ||
| arrowLongSide, | ||
| arrowOffsetFromEnd, | ||
| arrowOffsetFromBottom = 0 |
There was a problem hiding this comment.
II agree, let's add enough configuration for the cases we currently have, and if needed we can always expand the configuration later.
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))); | ||
| }); |
packages/scratch-gui/src/components/stage-header/stage-header.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements in-editor manual “Set Thumbnail” functionality by moving the control into the project editor’s stage header and adding supporting UI/UX (confirmation prompt, new alerts, and updated spinner/alert styling).
Changes:
- Adds a stage-header thumbnail button in the editor with a confirmation prompt and a “new feature” tooltip.
- Introduces new alert types/styling for “setting thumbnail”, “success”, and “error” states.
- Refactors thumbnail snapshot saving to be async-capable and exposes additional state selectors to gate UI.
Reviewed changes
Copilot reviewed 17 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/reducers/project-state.js | Adds getIsProjectLoadedWithId selector for gating the editor thumbnail UI. |
| packages/scratch-gui/src/lib/store-project-thumbnail.js | Refactors thumbnail snapshot flow to async/Promise-based behavior. |
| packages/scratch-gui/src/lib/assets/icon--error.svg | Adds an error icon asset for thumbnail failure alerts. |
| packages/scratch-gui/src/lib/alerts/index.jsx | Adds new thumbnail alerts + introduces INFO_BLUE alert level. |
| packages/scratch-gui/src/hooks/calculatePopupPosition.js | Adds a shared positioning helper used by tooltip/prompt popups. |
| packages/scratch-gui/src/css/colors.css | Adds new alert-related color variables. |
| packages/scratch-gui/src/containers/stage-header.jsx | Wires stage header to project load state + new alert dispatchers. |
| packages/scratch-gui/src/components/tooltip/tooltip.jsx | Adds a custom tooltip component for the “new feature” callout. |
| packages/scratch-gui/src/components/tooltip/tooltip.css | Styles for the new tooltip component. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-up.svg | Tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-right.svg | Tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-left.svg | Tooltip arrow asset. |
| packages/scratch-gui/src/components/tooltip/icon--arrow-down.svg | Tooltip arrow asset. |
| packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx | Passes userOwnsProject through to stage header. |
| packages/scratch-gui/src/components/stage-header/stage-header.jsx | Moves/adds “Set Thumbnail” button, confirmation prompt, tooltip, and alert integration. |
| packages/scratch-gui/src/components/stage-header/stage-header.css | Updates layout/styling for stage header control row. |
| packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg | Adds thumbnail button icon. |
| packages/scratch-gui/src/components/spinner/spinner.css | Adds spinner styling for info-blue alert level. |
| packages/scratch-gui/src/components/gui/gui.jsx | Adjusts prop wiring so stage wrapper receives manuallySaveThumbnails + userOwnsProject. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg | Confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg | Confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg | Confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg | Confirmation prompt arrow asset. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx | Adds a reusable confirmation prompt component for the thumbnail action. |
| packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css | Styles for the confirmation prompt. |
| packages/scratch-gui/src/components/button/button.jsx | Adds componentRef support so consumers can attach refs to the underlying <button>. |
| packages/scratch-gui/src/components/alerts/alert.css | Adds/updates alert styles for new levels and updated warning styling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => { | ||
| onUpdateProjectThumbnail( | ||
| projectId, | ||
| dataURItoBlob(dataURI), | ||
| resolve, | ||
| reject | ||
| ); | ||
| })); |
| export const storeProjectThumbnail = async (vm, callback) => { | ||
| try { | ||
| getProjectThumbnail(vm, callback); | ||
| await getProjectThumbnail(vm, callback); | ||
| } catch (e) { | ||
| 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; // re-throw so it is handled by alert | ||
| } |
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx
Outdated
Show resolved
Hide resolved
| align-self: stretch; | ||
| color: $ui-white; | ||
| text-align: center; | ||
| font-family: "Helvetica Neue"; |
There was a problem hiding this comment.
This is probably redundant
| text-align: center; | ||
| font-family: "Helvetica Neue"; | ||
| font-size: 1rem; | ||
| font-style: normal; |
There was a problem hiding this comment.
Nitpick: is this needed?
| } | ||
|
|
||
| .button-row { | ||
| font-weight: bolder; |
There was a problem hiding this comment.
Nitpick: would it be clearer to use a numeric value here as well?
| } | ||
|
|
||
| .button-row button:focus { | ||
| outline: auto; |
| relativeElementRef, | ||
| side, | ||
| align, | ||
| config |
There was a problem hiding this comment.
Nitpick: can we use a more precise name, such as layoutConfig?
| border-radius: 0.5rem; | ||
| border: 0.0625rem solid $motion-primary; | ||
| background-color: $motion-primary; | ||
| opacity: 1 !important; |
There was a problem hiding this comment.
Why do we need the important? Please leave as a comment in the code as well
| width: 21rem; | ||
| padding: 1rem; | ||
| align-items: flex-start; | ||
| gap: -0.0625rem; |
There was a problem hiding this comment.
Are you sure a negative gap is what you want? Or do you indeed want to reduce paddings / margins?
| .tooltip-title { | ||
| align-self: stretch; | ||
| color: $ui-white; | ||
| font-family: "Helvetica Neue"; |
|
|
||
| const updatePosition = useCallback(() => { | ||
| if (!targetRef?.current || !tooltipRef.current) return; | ||
| const newPos = calculatePopupPosition({ |
There was a problem hiding this comment.
I see a lot of similarities between the tooltip and the confirmation modal - the calls to calculatePopupPosition, the layout config, the arrowHeight / width. Do you see potential in extracting a common component between the two?
| }, [isOpen, updatePosition]); | ||
|
|
||
| // Click outside to close | ||
| useEffect(() => { |
There was a problem hiding this comment.
Such behaviors seem like we are implementing parts of the standard <Modal behaviour. Is there any potential in using a modal component underneath?
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