feat(game-editor): add Image Settings modal for flashcard images#71
feat(game-editor): add Image Settings modal for flashcard images#71pganesh-apphelix merged 3 commits intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an Image Settings modal to the Game Editor for managing alt text on flashcard images. When users click on a term or definition image in a flashcard, they can now edit the alt text through a dedicated modal interface that mirrors the UX from the Problem/Text editor.
Changes:
- Added alt text fields (
term_image_alt,definition_image_alt) to the game card data model across Redux state, API responses, and thunk actions - Implemented
GameImageSettingsModalcomponent with alt text controls and validation - Made flashcard images clickable to open the settings modal with keyboard accessibility support
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/editors/data/services/cms/api.ts | Added alt text fields to the flashcard API response mapping |
| src/editors/data/redux/thunkActions/game.js | Added alt text fields when loading and initializing game cards |
| src/editors/data/redux/game/reducers.js | Added alt text fields to initial state and created update actions for term and definition image alt text |
| src/editors/containers/GameEditor/messages.ts | Added internationalized messages for the image settings modal UI |
| src/editors/containers/GameEditor/index.jsx | Integrated the image settings modal, made images clickable, and wired up alt text save handlers |
| src/editors/containers/GameEditor/GameImageSettingsModal.scss | Added styles for the modal layout with thumbnail and controls |
| src/editors/containers/GameEditor/GameImageSettingsModal.jsx | Created the modal component with alt text controls and validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <IconButton | ||
| src={PictureIcon} | ||
| iconAs={Icon} | ||
| alt="IMG" | ||
| variant="plain" | ||
| /> |
There was a problem hiding this comment.
The tooltip is displaying an IconButton component instead of text, which is unusual for a tooltip. Tooltips should typically contain descriptive text. Consider using messages.imageSettingsTooltip which was defined but not used here.
| <IconButton | |
| src={PictureIcon} | |
| iconAs={Icon} | |
| alt="IMG" | |
| variant="plain" | |
| /> | |
| {intl.formatMessage(messages.imageSettingsTooltip)} |
|
|
||
| useEffect(() => { | ||
| if (imageData && isOpen) { | ||
| altText.setValue(imageData.altText || ''); | ||
| altText.setIsDecorative(!imageData.altText || imageData.altText === ''); | ||
| } | ||
| }, [imageData?.altText, imageData?.url, isOpen]); |
There was a problem hiding this comment.
The useEffect has dependencies on imageData?.altText and imageData?.url, but it also calls altText.setValue and altText.setIsDecorative. The altText object should be included in the dependency array to avoid stale closures, or the effect should be restructured to avoid this issue.
| useEffect(() => { | |
| if (imageData && isOpen) { | |
| altText.setValue(imageData.altText || ''); | |
| altText.setIsDecorative(!imageData.altText || imageData.altText === ''); | |
| } | |
| }, [imageData?.altText, imageData?.url, isOpen]); | |
| const { setValue, setIsDecorative } = altText; | |
| useEffect(() => { | |
| if (imageData && isOpen) { | |
| setValue(imageData.altText || ''); | |
| setIsDecorative(!imageData.altText || imageData.altText === ''); | |
| } | |
| }, [imageData?.altText, imageData?.url, isOpen, setValue, setIsDecorative]); |
67ed17e to
545d680
Compare
545d680 to
6e556aa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| altText: altText.isDecorative ? '' : altText.value, | ||
| isDecorative: altText.isDecorative, | ||
| }); | ||
| close(); |
There was a problem hiding this comment.
handleSave calls both onSave(...) and close(), but the parent onSave handler already closes the modal by resetting imageSettingsModal state. This results in redundant state updates and makes the close behavior harder to reason about. Let either the modal or the parent own closing (commonly: parent closes after a successful save, modal closes only on cancel).
| close(); |
| <IconButton | ||
| src={PictureIcon} | ||
| iconAs={Icon} | ||
| alt="IMG" | ||
| variant="plain" | ||
| /> |
There was a problem hiding this comment.
OverlayTrigger is rendering a Tooltip that contains an IconButton instead of tooltip text. This is likely not the intended UX (the tooltip will show an icon, not a label) and places an interactive control inside a tooltip, which is problematic for accessibility. Use the tooltip to render the localized label (e.g., messages.imageSettingsTooltip) and keep any icon/button outside the tooltip (or switch to IconButtonWithTooltip if you want an icon with a tooltip).
| <IconButton | |
| src={PictureIcon} | |
| iconAs={Icon} | |
| alt="IMG" | |
| variant="plain" | |
| /> | |
| {intl.formatMessage(messages.imageSettingsTooltip)} |
| <img | ||
| className="card-image" | ||
| src={imageUrl} | ||
| alt={`${imageType.toUpperCase()}_IMG`} |
There was a problem hiding this comment.
The card image alt attribute is still a placeholder (e.g., TERM_IMG) and does not use the saved alt text. This means the new alt-text setting won’t improve screen reader output. Use the stored altText value for alt (and an empty string when decorative) so the persisted alt text is actually applied.
| alt={`${imageType.toUpperCase()}_IMG`} | |
| alt={altText || ''} |
| <GameImageSettingsModal | ||
| key={`${imageSettingsModal.cardIndex}-${imageSettingsModal.imageType}-${imageSettingsModal.imageData?.url}`} | ||
| isOpen={imageSettingsModal.isOpen} | ||
| close={() => setImageSettingsModal({ | ||
| isOpen: false, | ||
| imageData: null, | ||
| cardIndex: null, | ||
| imageType: null, | ||
| })} | ||
| imageData={imageSettingsModal.imageData} | ||
| onSave={handleImageSettingsSave} | ||
| /> |
There was a problem hiding this comment.
New Image Settings behavior (opening the modal from the image and saving alt text back to Redux via updateTermImageAlt / updateDefinitionImageAlt) isn’t covered by tests. Since this file already has index.test.jsx covering image upload/delete, add tests that click the image to open the modal and verify that saving calls the correct update action with the entered alt text.
| imageSettingsTooltip: { | ||
| id: 'GameEditor.imageSettingsTooltip', | ||
| defaultMessage: 'Image settings', | ||
| description: 'Tooltip for image settings button.', | ||
| }, |
There was a problem hiding this comment.
imageSettingsTooltip is defined but not referenced anywhere. Either wire it into the image-settings hover/aria label (so it’s translated) or remove it to avoid dead i18n strings.
| imageSettingsTooltip: { | |
| id: 'GameEditor.imageSettingsTooltip', | |
| defaultMessage: 'Image settings', | |
| description: 'Tooltip for image settings button.', | |
| }, |
| term: card.term || '', | ||
| term_image: card.term_image || '', | ||
| term_image_path: card.term_image_path || '', | ||
| term_image_alt: card.term_image_alt || '', | ||
| definition: card.definition || '', | ||
| definition_image: card.definition_image || '', | ||
| definition_image_path: card.definition_image_path || '', | ||
| definition_image_alt: card.definition_image_alt || '', |
There was a problem hiding this comment.
After introducing *_image_alt fields, image removal/replacement can leave stale alt text in state (e.g., term_image_alt still set when term_image is cleared). Ensure the alt fields are cleared when an image is deleted (and ideally when a new image is uploaded) so saveGamesSettings doesn’t persist alt text for a non-existent/changed image.
| return { | ||
| ...baseCard, | ||
| term_image: card.term_image || '', | ||
| term_image_alt: card.term_image_alt || '', | ||
| definition_image: card.definition_image || '', | ||
| definition_image_alt: card.definition_image_alt || '', |
There was a problem hiding this comment.
saveGamesSettings always sends term_image_alt / definition_image_alt for flashcards, even when the corresponding image URL is empty. With the current delete/upload flows, this can persist stale alt text for a missing image. Consider normalizing the payload so the alt field is blank/omitted whenever its corresponding *_image is blank.
| return { | |
| ...baseCard, | |
| term_image: card.term_image || '', | |
| term_image_alt: card.term_image_alt || '', | |
| definition_image: card.definition_image || '', | |
| definition_image_alt: card.definition_image_alt || '', | |
| const termImage = card.term_image || ''; | |
| const definitionImage = card.definition_image || ''; | |
| return { | |
| ...baseCard, | |
| term_image: termImage, | |
| term_image_alt: termImage ? (card.term_image_alt || '') : '', | |
| definition_image: definitionImage, | |
| definition_image_alt: definitionImage ? (card.definition_image_alt || '') : '', |
Description
Jira
Acceptance Criteria
Screenshot