Skip to content
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.

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

Fix warnings in Problem Editor [FC-0062] #1280

Merged
5 changes: 5 additions & 0 deletions src/custom.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ declare module '*.svg' {
export default content;
}

declare module '*.png' {
const content: string;
export default content;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells typescript that when we import PNG files, that import is getting a string (from webpack, which is the URL to the image file)


declare module '*.json' {
const value: any;
export default value;
Expand Down
34 changes: 12 additions & 22 deletions src/editors/Editor.jsx → src/editors/Editor.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,29 @@
import React from 'react';
import { useDispatch } from 'react-redux';
import PropTypes from 'prop-types';
import { FormattedMessage } from '@edx/frontend-platform/i18n';

import messages from './messages';
import * as hooks from './hooks';

import supportedEditors from './supportedEditors';
import type { EditorComponent } from './EditorComponent';

const Editor = ({
export interface Props extends EditorComponent {
blockType: string;
blockId: string | null;
learningContextId: string | null;
lmsEndpointUrl: string | null;
studioEndpointUrl: string | null;
}

const Editor: React.FC<Props> = ({
learningContextId,
blockType,
blockId,
lmsEndpointUrl,
studioEndpointUrl,
onClose,
returnFunction,
onClose = null,
returnFunction = null,
}) => {
const dispatch = useDispatch();
hooks.initializeApp({
Expand Down Expand Up @@ -46,23 +54,5 @@ const Editor = ({
</div>
);
};
Editor.defaultProps = {
blockId: null,
learningContextId: null,
lmsEndpointUrl: null,
onClose: null,
returnFunction: null,
studioEndpointUrl: null,
};

Editor.propTypes = {
blockId: PropTypes.string,
blockType: PropTypes.string.isRequired,
learningContextId: PropTypes.string,
lmsEndpointUrl: PropTypes.string,
onClose: PropTypes.func,
returnFunction: PropTypes.func,
studioEndpointUrl: PropTypes.string,
};

export default Editor;
6 changes: 6 additions & 0 deletions src/editors/EditorComponent.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/** Shared interface that all Editor components (like ProblemEditor) adhere to */
export interface EditorComponent {
onClose: (() => void) | null;
// TODO: get a better type for the 'result' here
returnFunction?: (() => (result: any) => void) | null;
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import React from 'react';
import PropTypes from 'prop-types';
import { connect } from 'react-redux';

import { useDispatch, useSelector } from 'react-redux';
import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';
import {
ActionRow,
Button,
ModalDialog,
} from '@openedx/paragon';
import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import messages from './messages';
import * as hooks from '../hooks';

Expand All @@ -16,68 +15,55 @@ import { actions, selectors } from '../../../../../data/redux';
const SelectTypeFooter = ({
onCancel,
selected,
// redux
defaultSettings,
updateField,
setBlockTitle,
// injected,
intl,
}) => (
<div className="editor-footer fixed-bottom">
<ModalDialog.Footer className="border-top-0">
<ActionRow>
<ActionRow.Spacer />
<Button
aria-label={intl.formatMessage(messages.cancelButtonAriaLabel)}
variant="tertiary"
onClick={onCancel}
>
<FormattedMessage {...messages.cancelButtonLabel} />
</Button>
<Button
aria-label={intl.formatMessage(messages.selectButtonAriaLabel)}
onClick={hooks.onSelect({
selected,
updateField,
setBlockTitle,
defaultSettings,
})}
disabled={!selected}
>
<FormattedMessage {...messages.selectButtonLabel} />
</Button>
</ActionRow>
</ModalDialog.Footer>
</div>
);
}) => {
const intl = useIntl();
const defaultSettings = useSelector(selectors.problem.defaultSettings);
const dispatch = useDispatch();
const updateField = React.useCallback((data) => dispatch(actions.problem.updateField(data)), [dispatch]);
const setBlockTitle = React.useCallback((title) => dispatch(actions.app.setBlockTitle(title)), [dispatch]);
return (
<div className="editor-footer fixed-bottom">
<ModalDialog.Footer className="border-top-0">
<ActionRow>
<ActionRow.Spacer />
<Button
aria-label={intl.formatMessage(messages.cancelButtonAriaLabel)}
variant="tertiary"
onClick={onCancel}
>
<FormattedMessage {...messages.cancelButtonLabel} />
</Button>
<Button
aria-label={intl.formatMessage(messages.selectButtonAriaLabel)}
onClick={hooks.onSelect({
selected,
updateField,
setBlockTitle,
defaultSettings,
})}
disabled={!selected}
>
<FormattedMessage {...messages.selectButtonLabel} />
</Button>
</ActionRow>
</ModalDialog.Footer>
</div>
);
};

SelectTypeFooter.defaultProps = {
selected: null,
};

SelectTypeFooter.propTypes = {
defaultSettings: PropTypes.shape({
maxAttempts: PropTypes.number,
rerandomize: PropTypes.string,
showResetButton: PropTypes.bool,
showanswer: PropTypes.string,
}).isRequired,
// defaultSettings: PropTypes.shape({
// maxAttempts: PropTypes.number,
// rerandomize: PropTypes.string,
// showResetButton: PropTypes.bool,
// showanswer: PropTypes.string,
// }).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

These commented-out lines can be removed now that you're using selectors instead.

onCancel: PropTypes.func.isRequired,
selected: PropTypes.string,
updateField: PropTypes.func.isRequired,
setBlockTitle: PropTypes.func.isRequired,
// injected
intl: intlShape.isRequired,
};

export const mapStateToProps = (state) => ({
defaultSettings: selectors.problem.defaultSettings(state),
});

export const mapDispatchToProps = {
updateField: actions.problem.updateField,
setBlockTitle: actions.app.setBlockTitle,
};

export const SelectTypeFooterInternal = SelectTypeFooter; // For testing only
export default injectIntl(connect(mapStateToProps, mapDispatchToProps)(SelectTypeFooter));
export default SelectTypeFooter;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modernized this component by:

  • changing from propTypes to TypeScript Props
  • Changing injectIntl to useIntl
  • Changing connect to useSelector and useDispatch
  • Testing it in an actual browser in src/editors/containers/ProblemEditor/components/SelectTypeModal/index.test.tsx rather than shallow snapshot testing

There is no change to the actual code within the component itself.

Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
import 'CourseAuthoring/editors/setupEditorTest';
import React from 'react';
import { shallow } from '@edx/react-unit-test-utils';

import { Button } from '@openedx/paragon';
import { formatMessage } from '../../../../../testUtils';
import * as module from './SelectTypeFooter';
import SelectTypeFooter from './SelectTypeFooter';
import * as hooks from '../hooks';
import { actions } from '../../../../../data/redux';

const SelectTypeFooter = module.SelectTypeFooterInternal;

jest.mock('../hooks', () => ({
onSelect: jest.fn().mockName('onSelect'),
Expand Down Expand Up @@ -46,15 +42,4 @@ describe('SelectTypeFooter', () => {
.toEqual(expected);
});
});

describe('mapStateToProps', () => {
test('is empty', () => {
expect(module.mapDispatchToProps.defaultSettings).toEqual(actions.problem.defaultSettings);
});
});
describe('mapDispatchToProps', () => {
test('loads updateField from problem.updateField actions', () => {
expect(module.mapDispatchToProps.updateField).toEqual(actions.problem.updateField);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ exports[`SelectTypeWrapper snapshot 1`] = `
className="pgn__modal-close-container"
>
<IconButton
alt="Exit the editor"
Copy link
Contributor Author

@bradenmacdonald bradenmacdonald Sep 17, 2024

Choose a reason for hiding this comment

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

a11y fix: several of these icon buttons had no alt text!

iconAs="Icon"
src={[MockFunction icons.Close]}
/>
Expand All @@ -30,7 +31,7 @@ exports[`SelectTypeWrapper snapshot 1`] = `
test child
</h1>
</ModalDialog.Body>
<injectIntl(ShimmedIntlComponent)
<SelectTypeFooter
selected="iMAsElecTedValUE"
/>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React from 'react';
import PropTypes from 'prop-types';
import { injectIntl, FormattedMessage } from '@edx/frontend-platform/i18n';
import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';
import { Icon, ModalDialog, IconButton } from '@openedx/paragon';
import { Close } from '@openedx/paragon/icons';
import SelectTypeFooter from './SelectTypeFooter';

import * as hooks from '../../../../EditorContainer/hooks';
import ecMessages from '../../../../EditorContainer/messages';
import messages from './messages';

const SelectTypeWrapper = ({
Expand All @@ -14,6 +15,7 @@ const SelectTypeWrapper = ({
selected,
}) => {
const handleCancel = hooks.handleCancel({ onClose });
const intl = useIntl();

return (
<div
Expand All @@ -27,6 +29,7 @@ const SelectTypeWrapper = ({
src={Close}
iconAs={Icon}
onClick={handleCancel}
alt={intl.formatMessage(ecMessages.exitButtonAlt)}
/>
</div>
</ModalDialog.Title>
Expand All @@ -51,5 +54,4 @@ SelectTypeWrapper.propTypes = {
onClose: PropTypes.func,
};

export const SelectTypeWrapperInternal = SelectTypeWrapper; // For testing only
export default injectIntl(SelectTypeWrapper);
export default SelectTypeWrapper;
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import 'CourseAuthoring/editors/setupEditorTest';
import React from 'react';
import { shallow } from '@edx/react-unit-test-utils';
import { IconButton } from '@openedx/paragon';
import { SelectTypeWrapperInternal as SelectTypeWrapper } from '.';
import SelectTypeWrapper from '.';
import { handleCancel } from '../../../../EditorContainer/hooks';

jest.mock('../../../../EditorContainer/hooks', () => ({
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
import 'CourseAuthoring/editors/setupEditorTest';
import React from 'react';
import { shallow } from '@edx/react-unit-test-utils';

import { formatMessage } from '../../../../../testUtils';
import * as module from './AdvanceTypeSelect';

const AdvanceTypeSelect = module.AdvanceTypeSelectInternal;
import AdvanceTypeSelect from './AdvanceTypeSelect';

describe('AdvanceTypeSelect', () => {
const props = {
intl: { formatMessage },
selected: 'blankadvanced',
selected: 'blankadvanced' as const,
setSelected: jest.fn().mockName('setSelect'),
};
describe('snapshots', () => {
Expand All @@ -29,11 +24,6 @@ describe('AdvanceTypeSelect', () => {
shallow(<AdvanceTypeSelect {...props} selected="customgrader" />).snapshot,
).toMatchSnapshot();
});
test('snapshots: renders as expected with problemType is drag_and_drop', () => {
expect(
shallow(<AdvanceTypeSelect {...props} selected="drag_and_drop" />).snapshot,
).toMatchSnapshot();
});
test('snapshots: renders as expected with problemType is formularesponse', () => {
expect(
shallow(<AdvanceTypeSelect {...props} selected="formularesponse" />).snapshot,
Expand All @@ -44,14 +34,14 @@ describe('AdvanceTypeSelect', () => {
shallow(<AdvanceTypeSelect {...props} selected="imageresponse" />).snapshot,
).toMatchSnapshot();
});
test('snapshots: renders as expected with problemType is jsinput_response', () => {
test('snapshots: renders as expected with problemType is jsinputresponse', () => {
expect(
shallow(<AdvanceTypeSelect {...props} selected="jsinput_response" />).snapshot,
shallow(<AdvanceTypeSelect {...props} selected="jsinputresponse" />).snapshot,
).toMatchSnapshot();
});
test('snapshots: renders as expected with problemType is problem_with_hint', () => {
test('snapshots: renders as expected with problemType is problemwithhint', () => {
expect(
shallow(<AdvanceTypeSelect {...props} selected="problem_with_hint" />).snapshot,
shallow(<AdvanceTypeSelect {...props} selected="problemwithhint" />).snapshot,
).toMatchSnapshot();
});
});
Expand Down
Loading