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

feat: Course unit - Rendering xblocks in iframes #201

Merged
merged 18 commits into from
Mar 21, 2024

Conversation

PKulkoRaccoonGang
Copy link

@PKulkoRaccoonGang PKulkoRaccoonGang commented Mar 4, 2024

Dependencies

Edx-platform ts-develop branch

Description

This PR demonstrates the xblock content display functionality for the course unit page. The current implementation is a demonstration. Work on refactoring and displaying the Drag and Drop block continues.

The primary features were implemented:

  • Rendering xblocks inside iframes (Advanced, Discussion, Text, Open Response, Problem, Video).
  • The work of internal xblock scripts has been partially implemented.

Developer notes

  • The source for this functionality is frontend-app-library-authoring.
  • Technically, our goal is to visually display the content of the xblock + configure the work of the internal scripts of the xblock that are rendered inside the iframe (if it does not require a large investment of time).

Testing instructions

  1. Run master devstack.
  2. Start platform make dev.up.lms+cms+frontend-app-course-authoring and make checkout on this branch.
  3. Enable the new Unit page by adding a waffle flag contentstore.new_studio_mfe.use_new_unit_page in the CMS admin panel.
  4. Make sure that the MFE setting ENABLE_UNIT_PAGE=true is enabled.
  5. Сomment out the addition of the Ajax prefix in the edx-platform (temporary solution).
  6. Go to the Course Unit page from the Course Outline page.
Screen.Recording.2024-03-15.at.12.01.47.mov

@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as draft March 4, 2024 09:57
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/xblock-iframe-rendering branch from c6da976 to 4595dec Compare March 5, 2024 08:57
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/xblock-iframe-rendering branch from 9a76daf to 65f02cc Compare March 15, 2024 08:18
@PKulkoRaccoonGang PKulkoRaccoonGang changed the title feat: added xblock render engine feat: Course unit - Rendering xblocks in iframes Mar 15, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang self-assigned this Mar 15, 2024
@arbrandes
Copy link

Do you mind issuing this PR to the openedx org? That way it's clearer what's being changed in relation to master.

@PKulkoRaccoonGang
Copy link
Author

@arbrandes I don't mind, thank you

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/xblock-iframe-rendering branch from 88821ab to 839ce39 Compare March 20, 2024 12:37
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 13.52657% with 179 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (ts-develop@ed4532a). Click here to learn what that means.

Files Patch % Lines
...k-content/iframe-wrapper/tools/iframe-connector.js 0.00% 82 Missing and 14 partials ⚠️
...nit/course-xblock/xblock-content/XBlockContent.jsx 2.56% 38 Missing ⚠️
...ck/xblock-content/iframe-wrapper/iframe-wrapper.js 0.00% 27 Missing and 3 partials ⚠️
...block/xblock-content/iframe-wrapper/tools/utils.js 36.36% 3 Missing and 4 partials ⚠️
src/course-unit/data/api.js 57.14% 3 Missing ⚠️
src/course-unit/data/slice.js 0.00% 2 Missing ⚠️
src/course-unit/data/thunk.js 80.00% 2 Missing ⚠️
src/course-unit/course-xblock/CourseXBlock.jsx 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@              Coverage Diff              @@
##             ts-develop     #201   +/-   ##
=============================================
  Coverage              ?   88.02%           
=============================================
  Files                 ?      647           
  Lines                 ?    10988           
  Branches              ?     2280           
=============================================
  Hits                  ?     9672           
  Misses                ?     1256           
  Partials              ?       60           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/xblock-iframe-rendering branch from 839ce39 to b12ba83 Compare March 20, 2024 13:28
@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as ready for review March 20, 2024 13:39
@@ -34,12 +42,17 @@ const CourseXBlock = ({
const courseId = useSelector(getCourseId);
const canEdit = useSelector(getCanEdit);
const intl = useIntl();
const iframeUrl = getIFrameUrl({ blockId: id });
const xblockIframeHtmlAndResources = useSelector(getXBlockIframeHtmlAndResources);
const xblockInstanceHtmlAndResources = find(xblockIframeHtmlAndResources, { xblockId: id });
Copy link

Choose a reason for hiding this comment

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

Could explain more in detail how does xblockIframeHtmlAndResources look like?

Copy link
Author

Choose a reason for hiding this comment

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

image

};

// eslint-disable-next-line import/prefer-default-export
export const getHandlerUrl = async (blockId) => getXBlockHandlerUrl(blockId, 'handler_name');
Copy link

Choose a reason for hiding this comment

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

These two functions maybe we should move to the api.js?

Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] Do we need to create a wrapper function instead of a single function?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thanks
It makes sence

* The IFrame is resized responsively so that it fits the content height.
*
* We use an IFrame so that the XBlock code, including user-authored HTML,
* cannot access things like the user's cookies, nor can it make GET/POST
Copy link

Choose a reason for hiding this comment

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

I think this comment is not relevant. Maybe to reduce the size of it or just remove.

We can use user's cookies using our workaround.

Copy link
Author

Choose a reason for hiding this comment

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

Removed

};
const iframeRef = useRef(null);
const [html, setHtml] = useState(null);
const [iFrameHeight, setIFrameHeight] = useState(0);
Copy link

Choose a reason for hiding this comment

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

Suggested change
const [iFrameHeight, setIFrameHeight] = useState(0);
const [iframeHeight, setIFrameHeight] = useState(0);

Copy link
Author

Choose a reason for hiding this comment

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

Changed

const iframeRef = useRef(null);
const [html, setHtml] = useState(null);
const [iFrameHeight, setIFrameHeight] = useState(0);
const [iframeKey, setIframeKey] = useState(0);
Copy link

Choose a reason for hiding this comment

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

Suggested change
const [iframeKey, setIframeKey] = useState(0);
const [iframeKey, setIFrameKey] = useState(0);

Copy link
Author

Choose a reason for hiding this comment

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

Changed

* @param {string} itemId - The ID of the XBlock item.
* @returns {Promise<Object>} A Promise that resolves with the XBlock iframe data.
*/
export async function getXBlockIframeData(itemId) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
export async function getXBlockIframeData(itemId) {
export async function getXBlockIFrameData(itemId) {

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@@ -14,6 +14,7 @@ export const getSequenceId = (state) => state.courseUnit.sequenceId;
export const getCourseVerticalChildren = (state) => state.courseUnit.courseVerticalChildren;
export const getClipboardData = (state) => state.courseUnit.clipboardData;
const getLoadingStatuses = (state) => state.courseUnit.loadingStatus;
export const getXBlockIframeHtmlAndResources = (state) => state.courseUnit.xblockIframeHtmlAndResources;
Copy link

Choose a reason for hiding this comment

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

Suggested change
export const getXBlockIframeHtmlAndResources = (state) => state.courseUnit.xblockIframeHtmlAndResources;
export const getXBlockIFrameHtmlAndResources = (state) => state.courseUnit.xblockIframeHtmlAndResources;

Copy link
Author

Choose a reason for hiding this comment

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

Corrected

@@ -21,6 +21,7 @@ const slice = createSlice({
courseVerticalChildren: {},
clipboardData: null,
staticFileNotices: {},
xblockIframeHtmlAndResources: [],
Copy link

Choose a reason for hiding this comment

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

Suggested change
xblockIframeHtmlAndResources: [],
xblockIFrameHtmlAndResources: [],

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@@ -115,6 +116,9 @@ const slice = createSlice({
// This avoids the need to copy the array beforehand
state.courseVerticalChildren.children.sort((a, b) => (indexMap.get(a.id) || 0) - (indexMap.get(b.id) || 0));
},
fetchXBlockIframeResources: (state, { payload }) => {
Copy link

Choose a reason for hiding this comment

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

Suggested change
fetchXBlockIframeResources: (state, { payload }) => {
fetchXBlockIFrameResources: (state, { payload }) => {

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@@ -115,6 +116,9 @@ const slice = createSlice({
// This avoids the need to copy the array beforehand
state.courseVerticalChildren.children.sort((a, b) => (indexMap.get(a.id) || 0) - (indexMap.get(b.id) || 0));
},
fetchXBlockIframeResources: (state, { payload }) => {
state.xblockIframeHtmlAndResources.push(payload);
Copy link

Choose a reason for hiding this comment

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

Suggested change
state.xblockIframeHtmlAndResources.push(payload);
state.xblockIFrameHtmlAndResources.push(payload);

Copy link

Choose a reason for hiding this comment

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

And in all other cases either iframe or somethingIFrame

Copy link
Author

Choose a reason for hiding this comment

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

Done

package.json Outdated
@@ -88,7 +88,8 @@
"regenerator-runtime": "0.13.7",
"universal-cookie": "^4.0.4",
"uuid": "^3.4.0",
"yup": "0.31.1"
"yup": "0.31.1",
"querystring": "0.2.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] Do we need to install this package because I can't find usage of them?

Copy link
Author

Choose a reason for hiding this comment

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

Yes you are right
We don't use this package
Successfully deleted

};

// eslint-disable-next-line import/prefer-default-export
export const getHandlerUrl = async (blockId) => getXBlockHandlerUrl(blockId, 'handler_name');
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] Do we need to create a wrapper function instead of a single function?

Comment on lines +312 to +278
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.adding));

try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.adding));
try {
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.adding));
try {

Copy link
Author

Choose a reason for hiding this comment

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

We have this indentation in all sleds in this file
Do we need to delete it specifically in fetchXBlockIframeHtmlAndResourcesQuery?

@ihor-romaniuk
Copy link
Collaborator

ihor-romaniuk commented Mar 20, 2024

@PKulkoRaccoonGang [question] Should we cover our feature with unit tests in this PR?

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/xblock-iframe-rendering branch from 23003ab to 1f8eafc Compare March 20, 2024 20:10
@PKulkoRaccoonGang
Copy link
Author

PKulkoRaccoonGang commented Mar 20, 2024

@ihor-romaniuk Thank you for your attention 💯 We decided there will be no test coverage for now.

@monteri monteri merged commit e08b9e6 into ts-develop Mar 21, 2024
3 checks passed
khudym pushed a commit that referenced this pull request Mar 25, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
monteri pushed a commit that referenced this pull request Apr 1, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
ihor-romaniuk pushed a commit that referenced this pull request Apr 25, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
monteri pushed a commit that referenced this pull request Apr 29, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
PKulkoRaccoonGang added a commit that referenced this pull request Apr 30, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
PKulkoRaccoonGang pushed a commit that referenced this pull request Apr 30, 2024
fix: [AXIMST-27] Unit page - change iframe pararm for display xblock content (#151)

feat: [AXIMST-52] display unit and xblock render errors (#191)

* feat: [AXIMST-52] display unit and xblock render errors

* fix: after discussion

* fix: after review

feat: Course unit - Rendering xblocks in iframes (#201)

* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>

feat: [AXIMST-739] added xblock actions (#218)

* feat: [AXIMST-739] added xblock actions

* refactor: code refactoring

* refactor: refactoring after review

fix: after cherru-pick

fix: after cherry-pick

refactor: removed canEdit mock (#224)

fix: [AXIMST-655] fixed position of the view-port after loading the unit page (#217)

fix: [AXIMST-719] Course unit - Xblock problems (#213)

* fix: [AXIMST-719] fixed xblock problems

* fix: added asset and static replacer

* feat: added style extractor

* refactor: code refactoring

* refactor: after review

fix: [AXIMST-718] Course unit - Created logic for getting CSRF token (#226)

* fix: created logic for getting csrf token

* fix: usage of csrfTokenData

* refactor: code refactoring

---------

Co-authored-by: monteri <lansevermore>

fix: [AXIMST-745] added studioBaseUrl for static paths (#228)

refactor: [AXIMST-746] Unit page performance refactoring (#229)

Co-authored-by: Kyrylo Hudym-Levkovych <[email protected]>

fix: [AXIMST-785] fixed discard logic (#232)

feat: [AXIMST-800] Course unit - Added Collapse and Expand all buttons for xblocks (#234)

* feat: [AXIMST-800] added Collapse and Expand all buttons for xblocks

* feat: added tests

refactor: code refactoring
monteri pushed a commit that referenced this pull request May 3, 2024
* feat: added xblock render engine

* feat: connected iframe template

* feat: CSS and JS transfering

* feat: show video xblock

* feat: added multiply xblocks

* feat: rendering advanced components

* fix: add solution for request from within the iframe

* feat: iframe height

* refactor: code refactoring

* fix: fixed platform ajax prefix

* refactor: global refactoring

* refactor: show LTI xblock

* refactor: removed old xblock-content

* refactor: code refactoring

* refactor: some refactoring

* refactor: refactoring

* refactor: code refactoring

* refactor: refactoring after review

---------

Co-authored-by: monteri <lansevermore>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants