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

chore: iframe rendering optimization #1544

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.DS_Store
.eslintcache
.idea
.run
node_modules
npm-debug.log
coverage
Expand Down
16 changes: 11 additions & 5 deletions src/course-unit/CourseUnit.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ describe('<CourseUnit />', () => {
);

simulatePostMessageEvent(messageTypes.deleteXBlock, {
id: courseVerticalChildrenMock.children[0].block_id,
usageId: courseVerticalChildrenMock.children[0].block_id,
});

expect(getByText(/Delete this component?/i)).toBeInTheDocument();
Expand All @@ -257,10 +257,10 @@ describe('<CourseUnit />', () => {
const deleteButton = getAllByRole('button', { name: /Delete/i })
.find(({ classList }) => classList.contains('btn-primary'));

userEvent.click(cancelButton);
expect(cancelButton).toBeInTheDocument();

simulatePostMessageEvent(messageTypes.deleteXBlock, {
id: courseVerticalChildrenMock.children[0].block_id,
usageId: courseVerticalChildrenMock.children[0].block_id,
});

expect(getByRole('dialog')).toBeInTheDocument();
Expand Down Expand Up @@ -296,8 +296,12 @@ describe('<CourseUnit />', () => {

axiosMock
.onDelete(getXBlockBaseApiUrl(courseVerticalChildrenMock.children[0].block_id))
.replyOnce(200, { dummy: 'value' });
await executeThunk(deleteUnitItemQuery(courseId, blockId), store.dispatch);
.reply(200, { dummy: 'value' });
await executeThunk(deleteUnitItemQuery(
courseId,
courseVerticalChildrenMock.children[0].block_id,
simulatePostMessageEvent,
), store.dispatch);

const updatedCourseVerticalChildren = courseVerticalChildrenMock.children.filter(
child => child.block_id !== courseVerticalChildrenMock.children[0].block_id,
Expand Down Expand Up @@ -1617,6 +1621,8 @@ describe('<CourseUnit />', () => {
callbackFn: requestData.callbackFn,
}), store.dispatch);

simulatePostMessageEvent(messageTypes.rollbackMovedXBlock, { locator: requestData.sourceLocator });

const dismissButton = queryByRole('button', {
name: /dismiss/i, hidden: true,
});
Expand Down
1 change: 1 addition & 0 deletions src/course-unit/add-component/AddComponent.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const AddComponent = ({ blockId, handleCreateNewCourseXBlock }) => {
case COMPONENT_TYPES.problem:
case COMPONENT_TYPES.video:
handleCreateNewCourseXBlock({ type, parentLocator: blockId }, ({ courseKey, locator }) => {
localStorage.setItem('modalEditLastYPosition', window.scrollY);
navigate(`/course/${courseKey}/editor/${type}/${locator}`);
});
break;
Expand Down
8 changes: 7 additions & 1 deletion src/course-unit/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,20 @@ export const messageTypes = {
videoFullScreen: 'plugin.videoFullScreen',
refreshXBlock: 'refreshXBlock',
showMoveXBlockModal: 'showMoveXBlockModal',
completeXBlockMoving: 'completeXBlockMoving',
rollbackMovedXBlock: 'rollbackMovedXBlock',
showMultipleComponentPicker: 'showMultipleComponentPicker',
addSelectedComponentsToBank: 'addSelectedComponentsToBank',
showXBlockLibraryChangesPreview: 'showXBlockLibraryChangesPreview',
copyXBlock: 'copyXBlock',
manageXBlockAccess: 'manageXBlockAccess',
completeManageXBlockAccess: 'completeManageXBlockAccess',
deleteXBlock: 'deleteXBlock',
completeXBlockDeleting: 'completeXBlockDeleting',
duplicateXBlock: 'duplicateXBlock',
refreshXBlockPositions: 'refreshPositions',
completeXBlockDuplicating: 'completeXBlockDuplicating',
newXBlockEditor: 'newXBlockEditor',
toggleCourseXBlockDropdown: 'toggleCourseXBlockDropdown',
addXBlock: 'addXBlock',
scrollToXBlock: 'scrollToXBlock',
};
32 changes: 20 additions & 12 deletions src/course-unit/data/thunk.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { RequestStatus } from '../../data/constants';
import { NOTIFICATION_MESSAGES } from '../../constants';
import { updateModel, updateModels } from '../../generic/model-store';
import { updateClipboardData } from '../../generic/data/slice';
import { messageTypes } from '../constants';
import {
getCourseUnitData,
editUnitDisplayName,
Expand Down Expand Up @@ -126,6 +127,7 @@ export function editCourseUnitVisibilityAndData(
isVisible,
groupAccess,
isDiscussionEnabled,
callback,
blockId = itemId,
) {
return async (dispatch) => {
Expand All @@ -143,6 +145,9 @@ export function editCourseUnitVisibilityAndData(
isDiscussionEnabled,
).then(async (result) => {
if (result) {
if (callback) {
callback();
}
const courseUnit = await getCourseUnitData(blockId);
dispatch(fetchCourseItemSuccess(courseUnit));
const courseVerticalChildrenData = await getCourseVerticalChildren(blockId);
Expand All @@ -158,11 +163,8 @@ export function editCourseUnitVisibilityAndData(
};
}

export function createNewCourseXBlock(body, callback, blockId) {
export function createNewCourseXBlock(body, callback, blockId, sendMessageToIframe) {
return async (dispatch) => {
dispatch(updateLoadingCourseXblockStatus({ status: RequestStatus.IN_PROGRESS }));
dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));

if (body.stagedContent) {
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.pasting));
} else {
Expand All @@ -188,10 +190,10 @@ export function createNewCourseXBlock(body, callback, blockId) {
const courseVerticalChildrenData = await getCourseVerticalChildren(blockId);
dispatch(updateCourseVerticalChildren(courseVerticalChildrenData));
dispatch(hideProcessingNotification());
dispatch(updateLoadingCourseXblockStatus({ status: RequestStatus.SUCCESSFUL }));
dispatch(updateSavingStatus({ status: RequestStatus.SUCCESSFUL }));
if (callback) {
callback(result);
} else {
sendMessageToIframe(messageTypes.addXBlock, { data: result });
}
const currentBlockId = body.category === 'vertical' ? formattedResult.locator : blockId;
const courseUnit = await getCourseUnitData(currentBlockId);
Expand Down Expand Up @@ -220,13 +222,14 @@ export function fetchCourseVerticalChildrenData(itemId) {
};
}

export function deleteUnitItemQuery(itemId, xblockId) {
export function deleteUnitItemQuery(itemId, xblockId, sendMessageToIframe) {
return async (dispatch) => {
dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.deleting));

try {
await deleteUnitItem(xblockId);
sendMessageToIframe(messageTypes.completeXBlockDeleting, null);
const { userClipboard } = await getCourseSectionVerticalData(itemId);
dispatch(updateClipboardData(userClipboard));
const courseUnit = await getCourseUnitData(itemId);
Expand All @@ -240,13 +243,14 @@ export function deleteUnitItemQuery(itemId, xblockId) {
};
}

export function duplicateUnitItemQuery(itemId, xblockId) {
export function duplicateUnitItemQuery(itemId, xblockId, callback) {
return async (dispatch) => {
dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.duplicating));

try {
await duplicateUnitItem(itemId, xblockId);
const { courseKey, locator } = await duplicateUnitItem(itemId, xblockId);
callback(courseKey, locator);
const courseUnit = await getCourseUnitData(itemId);
dispatch(fetchCourseItemSuccess(courseUnit));
dispatch(hideProcessingNotification());
Expand Down Expand Up @@ -300,9 +304,13 @@ export function patchUnitItemQuery({
dispatch(updateMovedXBlockParams(xBlockParams));
dispatch(updateCourseOutlineInfo({}));
dispatch(updateCourseOutlineInfoLoadingStatus({ status: RequestStatus.IN_PROGRESS }));
const courseUnit = await getCourseUnitData(currentParentLocator);
dispatch(fetchCourseItemSuccess(courseUnit));
callbackFn();
try {
const courseUnit = await getCourseUnitData(currentParentLocator);
dispatch(fetchCourseItemSuccess(courseUnit));
} catch (error) {
handleResponseErrors(error, dispatch, updateSavingStatus);
}
callbackFn(sourceLocator);
} catch (error) {
handleResponseErrors(error, dispatch, updateSavingStatus);
} finally {
Expand Down
8 changes: 0 additions & 8 deletions src/course-unit/header-title/HeaderTitle.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import {
import ConfigureModal from '../../generic/configure-modal/ConfigureModal';
import { getCourseUnitData } from '../data/selectors';
import { updateQueryPendingStatus } from '../data/slice';
import { messageTypes } from '../constants';
import { useIframe } from '../context/hooks';
import messages from './messages';

const HeaderTitle = ({
Expand All @@ -28,15 +26,9 @@ const HeaderTitle = ({
const currentItemData = useSelector(getCourseUnitData);
const [isConfigureModalOpen, openConfigureModal, closeConfigureModal] = useToggle(false);
const { selectedPartitionIndex, selectedGroupsLabel } = currentItemData.userPartitionInfo;
const { sendMessageToIframe } = useIframe();

const onConfigureSubmit = (...arg) => {
handleConfigureSubmit(currentItemData.id, ...arg, closeConfigureModal);
// TODO: this artificial delay is a temporary solution
// to ensure the iframe content is properly refreshed.
setTimeout(() => {
sendMessageToIframe(messageTypes.refreshXBlock, null);
}, 1000);
};

const getVisibilityMessage = () => {
Expand Down
56 changes: 24 additions & 32 deletions src/course-unit/header-title/HeaderTitle.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,53 +60,45 @@ describe('<HeaderTitle />', () => {
it('render HeaderTitle component correctly', () => {
const { getByText, getByRole } = renderComponent();

waitFor(() => {
expect(getByText(unitTitle)).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
});
expect(getByText(unitTitle)).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
});

it('render HeaderTitle with open edit form', () => {
const { getByRole } = renderComponent({
isTitleEditFormOpen: true,
});

waitFor(() => {
expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toHaveValue(unitTitle);
expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
});
expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toHaveValue(unitTitle);
expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
});

it('calls toggle edit title form by clicking on Edit button', () => {
const { getByRole } = renderComponent();

waitFor(() => {
const editTitleButton = getByRole('button', { name: messages.altButtonEdit.defaultMessage });
userEvent.click(editTitleButton);
expect(handleTitleEdit).toHaveBeenCalledTimes(1);
});
const editTitleButton = getByRole('button', { name: messages.altButtonEdit.defaultMessage });
userEvent.click(editTitleButton);
expect(handleTitleEdit).toHaveBeenCalledTimes(1);
});

it('calls saving title by clicking outside or press Enter key', async () => {
it('calls saving title by clicking outside or press Enter key', () => {
const { getByRole } = renderComponent({
isTitleEditFormOpen: true,
});

waitFor(() => {
const titleField = getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage });
userEvent.type(titleField, ' 1');
expect(titleField).toHaveValue(`${unitTitle} 1`);
userEvent.click(document.body);
expect(handleTitleEditSubmit).toHaveBeenCalledTimes(1);

userEvent.click(titleField);
userEvent.type(titleField, ' 2[Enter]');
expect(titleField).toHaveValue(`${unitTitle} 1 2`);
expect(handleTitleEditSubmit).toHaveBeenCalledTimes(2);
});
const titleField = getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage });
userEvent.type(titleField, ' 1');
expect(titleField).toHaveValue(`${unitTitle} 1`);
userEvent.click(document.body);
expect(handleTitleEditSubmit).toHaveBeenCalledTimes(1);

userEvent.click(titleField);
userEvent.type(titleField, ' 2[Enter]');
expect(titleField).toHaveValue(`${unitTitle} 1 2`);
expect(handleTitleEditSubmit).toHaveBeenCalledTimes(2);
});

it('displays a visibility message with the selected groups for the unit', async () => {
Expand All @@ -125,7 +117,7 @@ describe('<HeaderTitle />', () => {
const visibilityMessage = messages.definedVisibilityMessage.defaultMessage
.replace('{selectedGroupsLabel}', 'Visibility group 1');

waitFor(() => {
await waitFor(() => {
expect(getByText(visibilityMessage)).toBeInTheDocument();
});
});
Expand All @@ -140,8 +132,8 @@ describe('<HeaderTitle />', () => {
await executeThunk(fetchCourseUnitQuery(blockId), store.dispatch);
const { getByText } = renderComponent();

waitFor(() => {
expect(getByText(messages.someVisibilityMessage.defaultMessage)).toBeInTheDocument();
await waitFor(() => {
expect(getByText(messages.commonVisibilityMessage.defaultMessage)).toBeInTheDocument();
});
});
});
13 changes: 9 additions & 4 deletions src/course-unit/hooks.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const useCourseUnit = ({ courseId, blockId }) => {
isVisible,
groupAccess,
isDiscussionEnabled,
() => sendMessageToIframe(messageTypes.completeManageXBlockAccess, null),
blockId,
));
closeModalFn();
Expand Down Expand Up @@ -113,15 +114,19 @@ export const useCourseUnit = ({ courseId, blockId }) => {
};

const handleCreateNewCourseXBlock = (body, callback) => (
dispatch(createNewCourseXBlock(body, callback, blockId))
dispatch(createNewCourseXBlock(body, callback, blockId, sendMessageToIframe))
);

const unitXBlockActions = {
handleDelete: (XBlockId) => {
dispatch(deleteUnitItemQuery(blockId, XBlockId));
dispatch(deleteUnitItemQuery(blockId, XBlockId, sendMessageToIframe));
},
handleDuplicate: (XBlockId) => {
dispatch(duplicateUnitItemQuery(blockId, XBlockId));
dispatch(duplicateUnitItemQuery(
blockId,
XBlockId,
(courseKey, locator) => sendMessageToIframe(messageTypes.completeXBlockDuplicating, { courseKey, locator }),
));
},
};

Expand All @@ -136,7 +141,7 @@ export const useCourseUnit = ({ courseId, blockId }) => {
currentParentLocator,
isMoving: false,
callbackFn: () => {
sendMessageToIframe(messageTypes.refreshXBlock, null);
sendMessageToIframe(messageTypes.rollbackMovedXBlock, { locator: sourceLocator });
window.scrollTo({ top: 0, behavior: 'smooth' });
},
}));
Expand Down
4 changes: 2 additions & 2 deletions src/course-unit/move-modal/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ export const useMoveModal = ({
title: state.sourceXBlockInfo.current.displayName,
currentParentLocator: blockId,
isMoving: true,
callbackFn: () => {
sendMessageToIframe(messageTypes.refreshXBlock, null);
callbackFn: (sourceLocator: string) => {
sendMessageToIframe(messageTypes.completeXBlockMoving, { locator: sourceLocator });
closeModal();
window.scrollTo({ top: 0, behavior: 'smooth' });
},
Expand Down
14 changes: 8 additions & 6 deletions src/course-unit/sidebar/PublishControls.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@

const handleCourseUnitDiscardChanges = () => {
closeDiscardModal();
dispatch(editCourseUnitVisibilityAndData(blockId, PUBLISH_TYPES.discardChanges));
// TODO: this artificial delay is a temporary solution
// to ensure the iframe content is properly refreshed.
setTimeout(() => {
sendMessageToIframe(messageTypes.refreshXBlock, null);
}, 1000);
dispatch(editCourseUnitVisibilityAndData(
blockId,
PUBLISH_TYPES.discardChanges,
null,
null,
null,
() => sendMessageToIframe(messageTypes.refreshXBlock, null),

Check warning on line 44 in src/course-unit/sidebar/PublishControls.jsx

View check run for this annotation

Codecov / codecov/patch

src/course-unit/sidebar/PublishControls.jsx#L44

Added line #L44 was not covered by tests
));
};

const handleCourseUnitPublish = () => {
Expand Down
Loading
Loading