Skip to content

Conversation

@asadali145
Copy link
Contributor

@asadali145 asadali145 commented Oct 27, 2022

Description

  • This PR fixes the issue in the TinyMCE editor in Studio.
  • TinyMCE was upgraded from v4.0.20 to v5.5.1 under [BB-6151] feat: upgrades tinyMCE v4.0.20 to tinyMCE v5.5.1 #30335.
  • With these changes, we have also changed the way we get the text from the TinyMCE editor. We are passing text as format instead of raw to get the content from the editor object.
    Screenshot 2022-10-25 at 3 20 21 PM
  • It returns plain text only. In the end, we just check for the plain text changes, and if plain text is not changed then the styling changes like bold, and italic text are not saved. Hyperlinks are also not working.
  • Changes will impact course authors.

Before these changes:

Screen.Recording.2022-10-27.at.3.22.01.PM.mov

After these changes:

Screen.Recording.2022-10-27.at.3.49.43.PM.mov

Supporting information

https://github.mit.edu/mitxonline/mitxonline-issues/issues/162

Testing instructions

  • Edit course unit in the studio using the TinyMCE editor as in the supporting videos.
  • Edit only the styles of the existing text and save the changes.
  • Changes should be saved.

Deadline

"None"

@asadali145 asadali145 force-pushed the asad/course-authoring-html-styling-fix branch from 6a7a44b to 2431880 Compare October 27, 2022 13:00
@asadali145 asadali145 marked this pull request as ready for review October 28, 2022 09:30
Copy link
Contributor

@rayzhou-bit rayzhou-bit left a comment

Choose a reason for hiding this comment

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

lgtm! I tested this with new editor and it looks fine.

visualEditor = this.getVisualEditor();
raw_content = visualEditor.getContent({
format: "text",
format: "raw",
Copy link
Contributor

Choose a reason for hiding this comment

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

This reverts a change in https://github.com/openedx/edx-platform/pull/30335/files#diff-bd3e3877ed43b0228f0227205afd052479229747ee194918aab121957b9b0edbR1393

@DubeySandeep @Agrendalath I don't suppose you could tell us why you made this change when upgrade TinyMCE to v5.5.1?

Copy link
Member

Choose a reason for hiding this comment

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

@pdpinch, TinyMCE docs replaced all occurrences of the raw format with text between v4 and v5. These formats are undocumented, and we mistakenly assumed that this is the new way to get/set content in TinyMCE v5.

cc: @tecoholic

Copy link
Contributor

Choose a reason for hiding this comment

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

@pdpinch What @Agrendalath mentioned in the previous comment. I made the change thinking that the TinyMCE has changed the values from raw to text as there were no references to raw as a value in the docs.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @asadali145.

👍

  • I tested this: checked that HTML changes are submitted correctly.
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@openedx-webhooks
Copy link

Although this pull request is already merged, I'm still watching it for updates.

There is nothing you have to do. No action is needed from your side. Thanks again for your contribution.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

asadali145 added a commit to mitodl/edx-platform that referenced this pull request Jan 5, 2023
During the upgrade to TinyMCE v5, we changed the content format to `text`.
However, it ignores changes in HTML tags. This reverts the format to `raw`.
pdpinch pushed a commit that referenced this pull request Jan 5, 2023
During the upgrade to TinyMCE v5, we changed the content format to `text`.
However, it ignores changes in HTML tags. This reverts the format to `raw`.
dimitri-hoareau-WEL pushed a commit to dimitri-hoareau-WEL/edx-platform that referenced this pull request Jan 25, 2023
…x#31500)

During the upgrade to TinyMCE v5, we changed the content format to `text`.
However, it ignores changes in HTML tags. This reverts the format to `raw`.
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Feb 9, 2023
bprosserc pushed a commit to uarcl/edx-platform that referenced this pull request Feb 10, 2023
…x#31500)

During the upgrade to TinyMCE v5, we changed the content format to `text`.
However, it ignores changes in HTML tags. This reverts the format to `raw`.
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Apr 11, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Apr 11, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request May 22, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Jun 14, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Aug 9, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Oct 11, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Oct 13, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Nov 17, 2023
sambapete added a commit to EDUlib/edx-platform that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants