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: TinyMCE plugin insert iframe #427

Merged

Conversation

johnvente
Copy link
Contributor

@johnvente johnvente commented Nov 29, 2023

Description

This PR adds a TinyMCE plugin that enables the insertion of an iframe with its respective width and height dimensions, along with additional properties. It is similar to the official plugin found at:
Page Embed Plugin - Tiny Documentation

The plugin contains two tabs:

  • General tab: This tab allows us to add:

    • Source URL: the URL of the iframe (required)
    • Size: This allows us to add the size of the iframe in pixels
      • Inline value: Allows us to add custom width and height (px format)
      • Big embed: Will add a value of 800px for the width and the height
      • Small embed: Will add a value of 100px for the width and the height
  • Advanced tab: This tab allows us to add metadata (optional) for the iframe

    • Name: name attribute for the iframe
    • Title: title attribute for the iframe
    • Long description URL: longdesc attribute for the iframe
    • Show iframe border: A black border for the iframe container (checkbox)
    • Enable scrollbar: enable scrollbar for the iframe (checkbox)

DEMO

demo.mp4

button-iframe

General tab Advanced tab

How to test it (Devstack)

You need to have this Waffle flag activated

new_core_editors.use_new_text_editor

Doc here

  1. Go to ~/workspace/frontend-app-course-authoring
 cd ~/workspace/frontend-app-course-authoring 
  1. Create a file called module.config.js with the following config
module.exports = {
  localModules: [
    {
      moduleName: "@edx/frontend-lib-content-components/dist",
      dir: "./frontend-lib-content-components",
      dist: "src",
    },
    {
      moduleName: "@edx/frontend-lib-content-components",
      dir: "./frontend-lib-content-components",
      dist: "src",
    },
  ],
};
  1. Clone frontend-lib-content-components
git fetch origin pull/427/head:jv/tiny-mce-plugin-iframe-embed
git checkout jv/tiny-mce-plugin-iframe-embed
  1. initialize frontend-app-course-authoring
npm start

Note: Due to jsdom is not supported by tinyMCE we are adding test only for the plugin
check more here

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 29, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Nov 29, 2023

Thanks for the pull request, @johnvente! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 30, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 30, 2023
@e0d
Copy link

e0d commented Nov 30, 2023

This is still a draft, but there are some failures.

@johnvente johnvente marked this pull request as ready for review December 1, 2023 14:45
@johnvente
Copy link
Contributor Author

@e0d Hi there! this is ready for review now, I've fixed the failures

@santiagosuarezedunext
Copy link

Thanks @johnvente for this implementation! Please add unit tests to ensure the robustness of the code.

@santiagosuarezedunext
Copy link

Hey @jmakowski1123, any suggestions on who might be a good fit to review this? 👍

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6945234) 90.57% compared to head (a33a140) 90.70%.
Report is 9 commits behind head on main.

Files Patch % Lines
...ceWidget/customTinyMcePlugins/embedIframePlugin.js 98.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   90.57%   90.70%   +0.12%     
==========================================
  Files         227      229       +2     
  Lines        4129     4206      +77     
  Branches      831      855      +24     
==========================================
+ Hits         3740     3815      +75     
- Misses        369      371       +2     
  Partials       20       20              

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

@johnvente
Copy link
Contributor Author

Hi @santiagosuarezedunext! I've added the unit tests for the plugin

@jmakowski1123
Copy link

@mphilbrick211 This one already went through Product Review :) https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3887071233/Problem+Authors+don+t+have+enough+options+to+create+the+content+they+would+like

Let's touch base on how to connect the updated product review process, as this is the preferred way forward (ie, proposals get product review and approval before the PR is submitted, which is exactly what happened here.)

@santiagosuarezedunext
Copy link

Hi @mphilbrick211 😊
Who do you think would be the right person to review this PR?

@johnvente johnvente force-pushed the jv/tiny-mce-plugin-iframe-embed branch from 631d919 to f471c5c Compare December 21, 2023 23:18
@mphilbrick211 mphilbrick211 requested a review from a team December 23, 2023 22:33
@mphilbrick211
Copy link

Hi @mphilbrick211 😊 Who do you think would be the right person to review this PR?

Hi @santiagosuarezedunext! The reviewing team would be @openedx/teaching-and-learning, but they are out for the holidays until January.

@santiagosuarezedunext
Copy link

Hi @mphilbrick211
I hope you had a fantastic holiday break. I wanted to check in about the reviewing team, @openedx/teaching-and-learning, being out until January. Is there any update or progress on the matter?

Your assistance is greatly appreciated. Thank you!

Best regards.

@mphilbrick211
Copy link

Thanks for flagging, @santiagosuarezedunext! @openedx/2u-tnl @openedx/teaching-and-learning flagging for you!

@johnvente
Copy link
Contributor Author

Hi there @santiagosuarezedunext! Do you think this implementation should it be in a waffle flag?

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

I tested this in my local environment using Tutor, and it works as described in the cover letter! Thank you. Let me know if anything else changes.

@johnvente
Copy link
Contributor Author

Hi there @navinkarkera! We know that you are not maintainer from this repository but could you take a look in this PR? That would help us a lot. Thank you very much!

@santiagosuarezedunext
Copy link

@johnvente, it's not necessary. Unlike other cases, this development received product approval without being flagged behind a waffle flag, and this is because it's an isolated improvement that doesn't affect any workflow or add anything new; it's just an enhancement to something that could already be done.
CC: @jmakowski1123 @mphilbrick211

@BryanttV
Copy link

Hi @johnvente, I also tested the implementation using tutor and it worked perfectly!

@brian-smith-tcril brian-smith-tcril merged commit 90d5ac4 into openedx:main Feb 16, 2024
7 checks passed
@openedx-webhooks
Copy link

@johnvente 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

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.