Skip to content

feat(game-editor): add Image Settings modal for flashcard images #71

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.

Sign up for GitHub

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

Merged
pganesh-apphelix merged 3 commits into from
Feb 27, 2026
Merged

feat(game-editor): add Image Settings modal for flashcard images #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a1fb3c
feat(game-editor): add Image Settings modal for flashcard images
pganesh-apphelix Feb 18, 2026
c451804
fix: update game tests for missing image alt fields
pganesh-apphelix Feb 23, 2026
6e556aa
fix: handle gamexblock MFE editor close action
pganesh-apphelix Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/course-outline/unit-card/UnitCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ const UnitCard = ({
blockId={editXBlockId}
studioEndpointUrl={getConfig().STUDIO_BASE_URL}
lmsEndpointUrl={getConfig().LMS_BASE_URL}
onClose={null}
onClose={handleCloseMFEEditor}
returnFunction={handleSaveEditedXBlockData}
/>
</div>
Expand Down
105 changes: 105 additions & 0 deletions src/editors/containers/GameEditor/GameImageSettingsModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import React, { useEffect } from 'react';
import PropTypes from 'prop-types';
import { Button, Image } from '@openedx/paragon';
import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';
import BaseModal from '../../sharedComponents/BaseModal';
import AltTextControls from '../../sharedComponents/ImageUploadModal/ImageSettingsModal/AltTextControls';
import ErrorAlert from '../../sharedComponents/ErrorAlerts/ErrorAlert';
import * as hooks from '../../sharedComponents/ImageUploadModal/ImageSettingsModal/hooks';
import messages from './messages';
import './GameImageSettingsModal.scss';

/**
* Simplified image settings modal for game editor flashcards
* Only includes alt-text controls, excludes width/height dimensions
* @param {bool} isOpen - is the modal open?
* @param {func} close - close the modal
* @param {obj} imageData - current image data object with url and altText
* @param {func} onSave - save callback with updated alt text
*/
const GameImageSettingsModal = ({
close,
isOpen,
imageData,
onSave,
}) => {
const intl = useIntl();
const altText = hooks.altTextHooks(imageData?.altText || '');

useEffect(() => {
if (imageData && isOpen) {
altText.setValue(imageData.altText || '');
altText.setIsDecorative(!imageData.altText || imageData.altText === '');
}
}, [imageData?.altText, imageData?.url, isOpen]);
Comment on lines +28 to +34
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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]);

Copilot uses AI. Check for mistakes.

const handleSave = () => {
if (!altText.isDecorative && !altText.value?.trim()) {
altText.error.set();
return;
}

onSave({
altText: altText.isDecorative ? '' : altText.value,
isDecorative: altText.isDecorative,
});
close();
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
close();

Copilot uses AI. Check for mistakes.
};

if (!imageData) {
return null;
}

return (
<BaseModal
close={close}
confirmAction={(
<Button
variant="primary"
onClick={handleSave}
>
<FormattedMessage {...messages.saveButtonLabel} />
</Button>
)}
isOpen={isOpen}
title={intl.formatMessage(messages.imageSettingsTitle)}
>
<ErrorAlert
dismissError={altText.error.dismiss}
hideHeading
isError={altText.error.show}
>
<FormattedMessage {...messages.altTextError} />
</ErrorAlert>
<div className="d-flex flex-row m-2 game-img-settings-container">
<div className="game-img-thumbnail-container">
<Image
className="game-img-thumbnail"
src={imageData.url}
alt={imageData.altText || ''}
/>
</div>
<hr className="h-100 bg-primary-200 m-0 mx-3" />
<div className="game-img-settings-controls">
<AltTextControls {...altText} />
</div>
</div>
</BaseModal>
);
};

GameImageSettingsModal.propTypes = {
close: PropTypes.func.isRequired,
isOpen: PropTypes.bool.isRequired,
imageData: PropTypes.shape({
url: PropTypes.string.isRequired,
altText: PropTypes.string,
}),
onSave: PropTypes.func.isRequired,
};

GameImageSettingsModal.defaultProps = {
imageData: null,
};

export default GameImageSettingsModal;
31 changes: 31 additions & 0 deletions src/editors/containers/GameEditor/GameImageSettingsModal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
.game-img-settings-container {
.game-img-thumbnail-container {
width: 250px;
display: flex;
align-items: center;
justify-content: center;

.game-img-thumbnail {
max-height: 250px;
max-width: 250px;
object-fit: contain;
}
}

hr {
width: 1px;
}

.game-img-settings-controls {
flex: 1;
min-width: 375px;

.img-settings-control-label {
font-size: 1rem;
}

.decorative-control-label label {
font-size: .75rem;
}
}
}
100 changes: 95 additions & 5 deletions src/editors/containers/GameEditor/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import Button from '../../sharedComponents/Button';
import DraggableList, { SortableItem } from '../../../generic/DraggableList';
import messages from './messages';
import PictureIcon from './PictureIcon';
import GameImageSettingsModal from './GameImageSettingsModal';

export const hooks = {
getContent: ({ type, settings, list }) => {
Expand Down Expand Up @@ -89,6 +90,8 @@ export const GameEditor = ({
list,
updateTerm,
updateDefinition,
updateTermImageAlt,
updateDefinitionImageAlt,
toggleOpen,
setList,
addCard,
Expand All @@ -106,6 +109,12 @@ export const GameEditor = ({
const [validationErrors, setValidationErrors] = useState({});
const [localInputValues, setLocalInputValues] = useState({});
const [isAlertVisible, setIsAlertVisible] = useState(true);
const [imageSettingsModal, setImageSettingsModal] = useState({
isOpen: false,
imageData: null,
cardIndex: null,
imageType: null,
});

const MAX_TERM_LENGTH = 120;
const MAX_DEFINITION_LENGTH = 120;
Expand Down Expand Up @@ -322,9 +331,56 @@ export const GameEditor = ({
</div>
);

const renderImageDisplay = useCallback((imageUrl, filePath, index, imageType) => (
const renderImageDisplay = useCallback((imageUrl, filePath, index, imageType, altText) => (
<div className="card-image-area d-flex align-items-center align-self-stretch">
<img className="card-image" src={imageUrl} alt={`${imageType.toUpperCase()}_IMG`} />
<OverlayTrigger
placement="bottom"
overlay={(
<Tooltip id={`image-settings-tooltip-${imageType}-${index}`}>
<IconButton
src={PictureIcon}
iconAs={Icon}
alt="IMG"
variant="plain"
/>
Comment on lines +340 to +345
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
<IconButton
src={PictureIcon}
iconAs={Icon}
alt="IMG"
variant="plain"
/>
{intl.formatMessage(messages.imageSettingsTooltip)}

Copilot uses AI. Check for mistakes.
Comment on lines +340 to +345
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
<IconButton
src={PictureIcon}
iconAs={Icon}
alt="IMG"
variant="plain"
/>
{intl.formatMessage(messages.imageSettingsTooltip)}

Copilot uses AI. Check for mistakes.
</Tooltip>
)}
>
<div
role="button"
tabIndex={0}
style={{ cursor: 'pointer', display: 'inline-block' }}
onClick={() => setImageSettingsModal({
isOpen: true,
imageData: {
url: imageUrl,
altText: altText || '',
},
cardIndex: index,
imageType,
})}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
setImageSettingsModal({
isOpen: true,
imageData: {
url: imageUrl,
altText: altText || '',
},
cardIndex: index,
imageType,
});
}
}}
>
<img
className="card-image"
src={imageUrl}
alt={`${imageType.toUpperCase()}_IMG`}
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
alt={`${imageType.toUpperCase()}_IMG`}
alt={altText || ''}

Copilot uses AI. Check for mistakes.
/>
</div>
</OverlayTrigger>
<IconButton
src={DeleteOutline}
iconAs={Icon}
Expand All @@ -333,7 +389,7 @@ export const GameEditor = ({
onClick={() => handleImageRemove(index, imageType, filePath)}
/>
</div>
), [handleImageRemove]);
), [handleImageRemove, intl]);

const renderImageUploadButton = useCallback((index, imageType) => (
<IconButton
Expand All @@ -345,9 +401,27 @@ export const GameEditor = ({
/>
), []);

const termImageDiv = (card, index) => renderImageDisplay(card.term_image, card.term_image_path, index, 'term');
const handleImageSettingsSave = useCallback((altTextData) => {
const { cardIndex, imageType } = imageSettingsModal;
const altText = altTextData.altText || '';

if (imageType === 'term') {
updateTermImageAlt({ index: cardIndex, termImageAlt: altText });
} else if (imageType === 'definition') {
updateDefinitionImageAlt({ index: cardIndex, definitionImageAlt: altText });
}

setImageSettingsModal({
isOpen: false,
imageData: null,
cardIndex: null,
imageType: null,
});
}, [imageSettingsModal, updateTermImageAlt, updateDefinitionImageAlt]);

const termImageDiv = (card, index) => renderImageDisplay(card.term_image, card.term_image_path, index, 'term', card.term_image_alt);
const termImageUploadButton = (card, index) => renderImageUploadButton(index, 'term');
const definitionImageDiv = (card, index) => renderImageDisplay(card.definition_image, card.definition_image_path, index, 'definition');
const definitionImageDiv = (card, index) => renderImageDisplay(card.definition_image, card.definition_image_path, index, 'definition', card.definition_image_alt);
const definitionImageUploadButton = (card, index) => renderImageUploadButton(index, 'definition');

const timerSettingsOption = (
Expand Down Expand Up @@ -705,6 +779,18 @@ export const GameEditor = ({
)}
{!blockFinished ? loading : page}
</div>
<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}
/>
Comment on lines +782 to +793
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
</EditorContainer>
);
};
Expand All @@ -720,6 +806,8 @@ GameEditor.propTypes = {
})).isRequired,
updateTerm: PropTypes.func.isRequired,
updateDefinition: PropTypes.func.isRequired,
updateTermImageAlt: PropTypes.func.isRequired,
updateDefinitionImageAlt: PropTypes.func.isRequired,
toggleOpen: PropTypes.func.isRequired,
setList: PropTypes.func.isRequired,
addCard: PropTypes.func.isRequired,
Expand Down Expand Up @@ -756,6 +844,8 @@ export const mapDispatchToProps = {
updateType: actions.game.updateType,
updateTerm: actions.game.updateTerm,
updateDefinition: actions.game.updateDefinition,
updateTermImageAlt: actions.game.updateTermImageAlt,
updateDefinitionImageAlt: actions.game.updateDefinitionImageAlt,
toggleOpen: actions.game.toggleOpen,
setList: actions.game.setList,
addCard: actions.game.addCard,
Expand Down
20 changes: 20 additions & 0 deletions src/editors/containers/GameEditor/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,26 @@ const messages = defineMessages({
defaultMessage: 'On',
description: 'Label for on toggle button.',
},
imageSettingsTooltip: {
id: 'GameEditor.imageSettingsTooltip',
defaultMessage: 'Image settings',
description: 'Tooltip for image settings button.',
},
Comment on lines +159 to +163
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
imageSettingsTooltip: {
id: 'GameEditor.imageSettingsTooltip',
defaultMessage: 'Image settings',
description: 'Tooltip for image settings button.',
},

Copilot uses AI. Check for mistakes.
imageSettingsTitle: {
id: 'GameEditor.imageSettingsTitle',
defaultMessage: 'Image Settings',
description: 'Title for image settings modal.',
},
saveButtonLabel: {
id: 'GameEditor.saveButtonLabel',
defaultMessage: 'Save',
description: 'Label for save button in image settings modal.',
},
altTextError: {
id: 'GameEditor.altTextError',
defaultMessage: 'Alt text is required for non-decorative images.',
description: 'Error message when alt text is missing.',
},
});

export default messages;
6 changes: 6 additions & 0 deletions src/editors/data/redux/game/reducers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ const initialState = {
term: '',
term_image: '',
term_image_path: '',
term_image_alt: '',
definition: '',
definition_image: '',
definition_image_path: '',
definition_image_alt: '',
editorOpen: true,
},
],
Expand Down Expand Up @@ -67,9 +69,11 @@ const game = createSlice({
term: '',
term_image: '',
term_image_path: '',
term_image_alt: '',
definition: '',
definition_image: '',
definition_image_path: '',
definition_image_alt: '',
editorOpen: true,
},
],
Expand Down Expand Up @@ -103,9 +107,11 @@ const actions = StrictDict({
updateTerm: ({ index, term }) => baseActions.updateCardField({ index, field: 'term', value: term }),
updateTermImage: ({ index, termImage }) => baseActions.updateCardField({ index, field: 'term_image', value: termImage }),
updateTermImagePath: ({ index, termImagePath }) => baseActions.updateCardField({ index, field: 'term_image_path', value: termImagePath }),
updateTermImageAlt: ({ index, termImageAlt }) => baseActions.updateCardField({ index, field: 'term_image_alt', value: termImageAlt }),
updateDefinition: ({ index, definition }) => baseActions.updateCardField({ index, field: 'definition', value: definition }),
updateDefinitionImage: ({ index, definitionImage }) => baseActions.updateCardField({ index, field: 'definition_image', value: definitionImage }),
updateDefinitionImagePath: ({ index, definitionImagePath }) => baseActions.updateCardField({ index, field: 'definition_image_path', value: definitionImagePath }),
updateDefinitionImageAlt: ({ index, definitionImageAlt }) => baseActions.updateCardField({ index, field: 'definition_image_alt', value: definitionImageAlt }),
toggleOpen: ({ index, isOpen }) => baseActions.updateCardField({ index, field: 'editorOpen', value: !!isOpen }),
});

Expand Down
4 changes: 4 additions & 0 deletions src/editors/data/redux/game/reducers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ describe('game reducer', () => {
term: '',
term_image: '',
term_image_path: '',
term_image_alt: '',
definition: '',
definition_image: '',
definition_image_path: '',
definition_image_alt: '',
editorOpen: true,
}],
isDirty: false,
Expand Down Expand Up @@ -225,9 +227,11 @@ describe('game reducer', () => {
term: '',
term_image: '',
term_image_path: '',
term_image_alt: '',
definition: '',
definition_image: '',
definition_image_path: '',
definition_image_alt: '',
editorOpen: true,
});
expect(result.isDirty).toBe(true);
Expand Down
Loading