feat: insert link plugin tinymce #455

johnvente · 2024-01-18T02:37:12Z

Description

This plugin allows you to insert a link into the text editor. Additionally, you can search for sections, subsections, and units within the course and insert the URL for any of them

Demo

demo-jum-to.mov

How to test it (Devstack)

You need to have this Waffle flag activated

new_core_editors.use_new_text_editor

Create waffle flag

Doc here

Go to ~/workspace/frontend-app-course-authoring

 cd ~/workspace/frontend-app-course-authoring

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",
    },
  ],
};

Clone frontend-lib-content-components inside frontend-app-course-authoring

git clone -b jv/feat-tiny-mce-plugin-share-link https://github.com/eduNEXT/frontend-lib-content-components.git
cd frontend-lib-content-components && nvm use && npm install

If you have cloned frontend-lib-content-components already you can do this

cd frontend-lib-content-components && git fetch origin pull/455/head:jv/feat-tiny-mce-plugin-share-link
git checkout jv/feat-tiny-mce-plugin-share-link
nvm use && npm install

initialize frontend-app-course-authoring

cd ~/workspace/frontend-app-course-authoring  && npm start

openedx-webhooks · 2024-01-18T02:37:17Z

Thanks for the pull request, @johnvente!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2024-01-18T03:05:08Z

Codecov Report

Attention: Patch coverage is 85.88710% with 35 lines in your changes missing coverage. Please review.

Project coverage is 89.07%. Comparing base (90d5ac4) to head (989b538).
Report is 53 commits behind head on main.

Files with missing lines	Patch %	Lines
...editors/sharedComponents/InsertLinkModal/index.jsx	69.11%	20 Missing and 1 partial ⚠️
...rc/editors/sharedComponents/TinyMceWidget/hooks.js	63.63%	7 Missing and 1 partial ⚠️
...redComponents/InsertLinkModal/BlocksList/index.jsx	88.88%	4 Missing ⚠️
...dComponents/InsertLinkModal/SearchBlocks/index.jsx	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   90.70%   89.07%   -1.64%     
==========================================
  Files         229      263      +34     
  Lines        4206     4721     +515     
  Branches      855      974     +119     
==========================================
+ Hits         3815     4205     +390     
- Misses        371      487     +116     
- Partials       20       29       +9

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

src/editors/sharedComponents/InsertLinkModal/BlocksList/index.jsx

src/editors/sharedComponents/InsertLinkModal/BlockLink/index.test.jsx

src/editors/sharedComponents/InsertLinkModal/BlocksList/index.jsx

mariajgrimaldi

All my comments were already addressed, thank you!

santiagosuarezedunext · 2024-02-14T16:59:56Z

To Whom It May Concern: This functionality was approved by product in this post

brian-smith-tcril

Overall this looks really great! It's super exciting to see this functionality built out.

I left a couple comments about possibly moving some things out of utils.js, as well as a comment with some questions about URL validation.

Thank you so much for this PR!

brian-smith-tcril · 2024-02-14T19:42:45Z

src/editors/sharedComponents/TinyMceWidget/index.jsx

+      {isInsertLinkOpen && (
+        <InsertLinkModal
+          isOpen={isInsertLinkOpen}


It looks like the rest of the modals handle isOpen entirely internally. Does this still work without the isInsertLinkOpen &&?

it does work without isInsertLinkOpen && the main problem is that the status of the modal keeps the same even if I open or close it which means for instance I selected an url that is inside the tree (an unit for example) and I open it again and it shows the link selected without that does not re-render the modal component

src/editors/sharedComponents/TinyMceWidget/hooks.js

brian-smith-tcril · 2024-02-14T20:40:17Z

src/editors/sharedComponents/InsertLinkModal/utils.js

In general I'm not a huge fan of having things live in a utils file (https://youtu.be/-J3wNP6u5YU?t=328 explains my reasoning behind that quite well). I'm not opposed to keeping this file if need be, but I'll go through it and see if I have any suggestions for what may be better homes for the things it contains.

src/editors/sharedComponents/InsertLinkModal/utils.js

johnvente · 2024-02-15T14:13:01Z

Overall this looks really great! It's super exciting to see this functionality built out.

I left a couple comments about possibly moving some things out of utils.js, as well as a comment with some questions about URL validation.

Thank you so much for this PR!

Hi @brian-smith-tcril! Thanks for taking a look here. I disagree with moving utils.js, principally the functions, because it's better to have functions that are reusable outside of a component. That way, you could use them in other parts of the code, and for testing purposes, it's easier to test those functions if they are outside a component. It would be tricky to test if they were inside a component, as discussed in this article. I understand that if for now they are only used inside one component, you might want to keep them there. I could move the functions to their respective component folders instead of having only one utils file, so each component will have its own utils

brian-smith-tcril · 2024-02-15T16:05:47Z

I could move the functions to their respective component folders instead of having only one utils file, so each component will have its own utils

This sounds like a reasonable solution to me. For example, the getChildrenFromList method could move from src/editors/sharedComponents/insertLinkModal/utils.js to src/editors/sharedComponents/insertLinkModal/BlocksList/utils/getChildrenFromList.js, and formatBlockPath could move to src/editors/sharedComponents/insertLinkModal/utils/formatBlockPath.js

That would effectively communicate the current usage scope of each function, and it would make it easy for someone who wants to use one more generally to refactor (by simply moving the file and changing an import path)

brian-smith-tcril · 2024-02-15T16:36:52Z

@johnvente it looks like the CI failures should be resolved if you rebase on latest main (it's @edx vs @openedx stuff that changed in #457)

brian-smith-tcril · 2024-04-02T17:36:26Z

Yep, but why to check the insertion bar?

Because that's how the native link button behaves. It doesn't make sense to have different UX for the internal link button and the native link button.

johnvente · 2024-04-02T17:55:12Z

Yep, but why to check the insertion bar?

Because that's how the native link button behaves. It doesn't make sense to have different UX for the internal link button and the native link button.

Okay, I will check that would it be enough with that?

brian-smith-tcril · 2024-04-02T18:03:31Z

I found another issue.

Select a link
Open the "jump to" editor
Click the "remove link" button
Click save
The link is still there

johnvente · 2024-04-02T21:35:36Z

I found another issue.

@brian-smith-tcril I added a fix for that use case but for the insertion bar not yet since the native plugin is active even when there is not text selected or when the editor is empty completely so is active all the time we should limit our plugin only for text selected instead of trying to have the behavior of the native plugin I'm not sure how long can take and I won't be able to check it after this friday please let me know what do you recommend here

brian-smith-tcril · 2024-04-03T01:15:44Z

since the native plugin is active even when there is not text selected

The grey indicates it will edit the link instead of create a new one

vs

I won't be able to check it after this friday

I'm more than happy to take this over and get it across the finish line.

johnvente · 2024-04-03T15:34:00Z

I'm more than happy to take this over and get it across the finish line.

That is more like new feature that a bug fixing since our code was made for text selected not when the user has insertion bar to edit a link. I will try as much as I can to check it but the time is very short

johnvente · 2024-04-05T15:41:05Z

Hi @brian-smith-tcril I added the support to edit a new link as you mentioned here could you take a look please? I will be checking it

brian-smith-tcril · 2024-04-05T16:36:57Z

Found a couple more issues

Use "jump to" button to create a link
Insertion bar and selection is now completely gone from the editor
Button state is now both disabled and has the grey box

compared to

Use native link button to create link
Selection stays, insertion bar stays, native link button state updated correctly

A similar thing with the button state happens when deleting a link using the jump to button

Also

Create link using native link editor (to an external site)
Select the link (or place insertion bar in it)
See "jump to" button in edit state
Click on "jump to" button
No page is navigated to (since it's an external link)

And another one that's a bit more complicated

Create a link using the native link editor (to page url copied by opening a "jump to" link in the native editor)
Select the link (or place insertion bar in it)
Clock on the "jump to" button
See no page selected in the jump to menu

I realize this last one is because the url isn't being parsed to determine if the link is internal or not, but instead an id is being associated with it - but it leads to confusing UX.

johnvente · 2024-04-05T22:14:01Z

Sorry for the delay here, I won't be able to make changes here but any request please let know to @felipemontoya @mariajgrimaldi

Good luck

brian-smith-tcril · 2024-04-05T22:18:17Z

@johnvente thank you for all of your work on this! I plan on taking this over at the beginning of next week. I know it's turned out to be more complicated than expected, and you've done an amazing job getting it to this point. I'm really happy to be able to use this as a base to get it across the finish line.

Thanks again and all the best!

brian-smith-tcril · 2024-04-08T14:09:33Z

src/editors/sharedComponents/TinyMceWidget/hooks.js

+      editor.on('SelectionChange', () => {
+        const node = editor.selection.getNode();
+        const isLink = node.nodeName === 'A';
+        const hasTextSelected = editor.selection.getContent().length > 0;
+        api.setActive(isLink);
+        api.setDisabled(!isLink && !hasTextSelected);
+      });


Suggested change

editor.on('SelectionChange', () => {

const node = editor.selection.getNode();

const isLink = node.nodeName === 'A';

const hasTextSelected = editor.selection.getContent().length > 0;

api.setActive(isLink);

api.setDisabled(!isLink && !hasTextSelected);

});

editor.on('SelectionChange', () => {

const node = editor.selection.getNode();

const isLink = node.nodeName === 'A';

let isInternalLink = false;

if (isLink) {

const selectedHTML = editor.selection.getContent({ format: 'html' }) || node.outerHTML;

const regexDataBlockId = new RegExp(/data-block-id="([^"]+)"/);

isInternalLink = regexDataBlockId.test(selectedHTML);

}

const hasTextSelected = editor.selection.getContent().length > 0;

api.setActive(isInternalLink);

api.setDisabled(!isInternalLink && !hasTextSelected);

});

By doing this I was able to get it so the "jump to" button is only enabled when trying to edit internal links, but I was able to break it decently quickly (and uncovered another bug in the process).

The native link editor doesn't remove data-block-id, so by

Creating an internal link using the "jump to" dialog

Editing that internal link and changing it to be an external link using the native link dialog

We end up with a link on the page with an associated block id even though the url doesn't match anymore.

This gets cleaned up when opening the "jump to" menu during the block id/url validation, but I'd like to clean it up before getting in there.

I'm looking into ways to do that without duplicating too much code.

santiagosuarezedunext · 2024-04-15T16:52:27Z

Hi @brian-smith-tcril, I hope you're doing well. I wanted to ask how the development of this feature is going. What is still needed to finish closing it? Do you need help with anything?

Best regards

brian-smith-tcril · 2024-04-16T11:31:52Z

@santiagosuarezedunext I have been focused on the edx-platform node 18 upgrade. I'll get back to this once I've worked through the issues with that.

santiagosuarezedunext · 2024-06-04T15:18:14Z

Hello @brian-smith-tcril, I hope you are well.
I wrote to you to follow up on the functionality. Is there any update on this? What is needed to finish it? Please tell me if anything is needed from our side.

brian-smith-tcril · 2024-06-07T17:59:24Z

@santiagosuarezedunext I have been busy with Redwood and haven't had time to dive into this, but I have deployed the current code from this PR on a sandbox here: https://studio.pr-1070-e8373e.sandboxes.opencraft.hosting/

The current implementation differs significantly from what was originally proposed and approved here: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3887071233/Problem+Authors+don+t+have+enough+options+to+create+the+content+they+would+like?focusedCommentId=3932585997

To move forward with this, the implementation deployed to the sandbox should go through Product and UX/UI testing. That testing should result in updated requirements for getting this PR into a mergeable state.

If I were to perform the Product/UX/UI testing, I would not pass the current implementation because:

There should not be 2 "add link" buttons, adding external and internal "jump to" links should be possible using the same button

If, however, someone decides that having 2 link buttons is acceptable, I would still not pass the current implementation because:

There should always be a way to add a link without selecting text
- This is currently only possible with external links
The internal "jump to" link button should not allow opening the modal to try to edit external links
- The internal "jump to" link button currently opens to "search course pages" when an external link is opened with it.
Adding a link to an internal page by pasting a URL into the external link editor, then opening that link in the internal "jump to" editor, should result in the correct internal page being selected
- The internal "jump to" link button currently opens to "search course pages" when an internal link added via URL using the external link editor is opened with it.
The internal "jump to" link button is currently using an icon associated with "open in new tab"

@ali-hugo tagging you for visibility. I'm going to put this PR in draft mode until Product and UI has determined the best path forward.

ali-hugo · 2024-06-10T11:46:46Z

@brian-smith-tcril Thanks for putting this on my radar.

I wanted to check out the sandbox so created an account, but I don't seem to be receiving a verification mail. Is it possible for you to verify my email address on your side?

santiagosuarezedunext · 2024-06-11T17:35:34Z

@brian-smith-tcril i have the same error that @ali-hugo have.

brian-smith-tcril · 2024-06-11T18:06:06Z

@santiagosuarezedunext I have marked your user as "Active" and "Staff." Could you try again and let me know if you're still having problems logging in?

Thank you for your patience!

santiagosuarezedunext · 2024-06-18T14:54:09Z

It already works for me, thanks @brian-smith-tcril

KristinAoki · 2024-08-27T14:14:24Z

Hi! Thank you for your contribution to this repo. This repo is being deprecated and all the code is being moved to frontend-app-course-authoring. If you would like your work to be included in the transition please resolve outstanding requests for this PR. This PR will automatically close once frontend-app-course-authoring PR #1208 is merged.

bradenmacdonald · 2024-08-29T19:07:29Z

Closing as we've now moved this code, per the notice above. Please re-open this PR against https://github.com/openedx/frontend-app-course-authoring/ if you'd like to continue it. Feel free to reach out to me if you aren't sure how to do that using git and I can help (basically: add your local content-components repo as a remote in course-authoring, then pull the branch and rebase it).

openedx-webhooks · 2024-08-29T19:07:31Z

@johnvente Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 18, 2024

feat: insert link plugin tinymce

53ce3a6

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from d3cded8 to 53ce3a6 Compare January 18, 2024 03:01

johnvente added 2 commits January 18, 2024 16:02

test: adding more test unit for insert link modal

e9dee15

test: adding test for insertlinkmodaltoggle hook

15f00a8

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from 6125a8d to 15f00a8 Compare January 18, 2024 21:26

johnvente added 3 commits January 18, 2024 16:55

test: some tests for formatblocks util

2dcc8a6

test: more test for insertlinkmodal component

214ea4d

test: adding more coverage for insertlinkmodal components

173e9f3

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from 25f0815 to 173e9f3 Compare January 19, 2024 19:49

johnvente marked this pull request as ready for review January 19, 2024 19:54

mariajgrimaldi reviewed Jan 19, 2024

View reviewed changes

src/editors/sharedComponents/InsertLinkModal/BlocksList/index.jsx Outdated Show resolved Hide resolved

refactor: subsectios name for blocklist component

3eebebb

dcoa reviewed Jan 22, 2024

View reviewed changes

src/editors/sharedComponents/InsertLinkModal/BlockLink/index.test.jsx Outdated Show resolved Hide resolved

fix: remove enzyme for tests

9cd8d8b

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from 896a584 to 9cd8d8b Compare January 22, 2024 14:05

AuraAlba reviewed Jan 24, 2024

View reviewed changes

src/editors/sharedComponents/InsertLinkModal/BlocksList/index.jsx Outdated Show resolved Hide resolved

refactor: remove blank line

b7b42e6

mariajgrimaldi approved these changes Feb 8, 2024

View reviewed changes

johnvente requested a review from brian-smith-tcril February 14, 2024 18:31

brian-smith-tcril reviewed Feb 14, 2024

View reviewed changes

refactor: moving utils to their components and url validation

c963cfa

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from 2429b3e to c963cfa Compare February 15, 2024 16:21

chore: upgrade with main branch

efc7dc8

fix: removing delete jump to link

b11bf08

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from edbbd18 to b11bf08 Compare April 2, 2024 21:29

johnvente added 2 commits April 5, 2024 10:01

feat: adding support for editing a selected link

b5b40b4

fix: test for shared components

989b538

johnvente force-pushed the jv/feat-tiny-mce-plugin-share-link branch from 16b54a3 to 989b538 Compare April 5, 2024 15:33

brian-smith-tcril reviewed Apr 8, 2024

View reviewed changes

brian-smith-tcril mentioned this pull request Jun 5, 2024

test tinymce link plugin openedx/frontend-app-authoring#1070

Draft

brian-smith-tcril marked this pull request as draft June 7, 2024 17:59

bradenmacdonald closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: insert link plugin tinymce #455

feat: insert link plugin tinymce #455

johnvente commented Jan 18, 2024 •

edited

Loading

openedx-webhooks commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

mariajgrimaldi left a comment

santiagosuarezedunext commented Feb 14, 2024

brian-smith-tcril left a comment

brian-smith-tcril Feb 14, 2024

johnvente Feb 15, 2024

brian-smith-tcril Feb 14, 2024

johnvente commented Feb 15, 2024

brian-smith-tcril commented Feb 15, 2024 •

edited

Loading

brian-smith-tcril commented Feb 15, 2024

brian-smith-tcril commented Apr 2, 2024

johnvente commented Apr 2, 2024

brian-smith-tcril commented Apr 2, 2024

johnvente commented Apr 2, 2024

brian-smith-tcril commented Apr 3, 2024

johnvente commented Apr 3, 2024

johnvente commented Apr 5, 2024

brian-smith-tcril commented Apr 5, 2024

johnvente commented Apr 5, 2024

brian-smith-tcril commented Apr 5, 2024

brian-smith-tcril Apr 8, 2024

santiagosuarezedunext commented Apr 15, 2024

brian-smith-tcril commented Apr 16, 2024

santiagosuarezedunext commented Jun 4, 2024

brian-smith-tcril commented Jun 7, 2024

ali-hugo commented Jun 10, 2024

santiagosuarezedunext commented Jun 11, 2024

brian-smith-tcril commented Jun 11, 2024

santiagosuarezedunext commented Jun 18, 2024

KristinAoki commented Aug 27, 2024

bradenmacdonald commented Aug 29, 2024

openedx-webhooks commented Aug 29, 2024

feat: insert link plugin tinymce #455

feat: insert link plugin tinymce #455

Conversation

johnvente commented Jan 18, 2024 • edited Loading

Description

Demo

How to test it (Devstack)

openedx-webhooks commented Jan 18, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Update the status of your PR

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

mariajgrimaldi left a comment

Choose a reason for hiding this comment

santiagosuarezedunext commented Feb 14, 2024

brian-smith-tcril left a comment

Choose a reason for hiding this comment

brian-smith-tcril Feb 14, 2024

Choose a reason for hiding this comment

johnvente Feb 15, 2024

Choose a reason for hiding this comment

brian-smith-tcril Feb 14, 2024

Choose a reason for hiding this comment

johnvente commented Feb 15, 2024

brian-smith-tcril commented Feb 15, 2024 • edited Loading

brian-smith-tcril commented Feb 15, 2024

brian-smith-tcril commented Apr 2, 2024

johnvente commented Apr 2, 2024

brian-smith-tcril commented Apr 2, 2024

johnvente commented Apr 2, 2024

brian-smith-tcril commented Apr 3, 2024

johnvente commented Apr 3, 2024

johnvente commented Apr 5, 2024

brian-smith-tcril commented Apr 5, 2024

johnvente commented Apr 5, 2024

brian-smith-tcril commented Apr 5, 2024

brian-smith-tcril Apr 8, 2024

Choose a reason for hiding this comment

santiagosuarezedunext commented Apr 15, 2024

brian-smith-tcril commented Apr 16, 2024

santiagosuarezedunext commented Jun 4, 2024

brian-smith-tcril commented Jun 7, 2024

ali-hugo commented Jun 10, 2024

santiagosuarezedunext commented Jun 11, 2024

brian-smith-tcril commented Jun 11, 2024

santiagosuarezedunext commented Jun 18, 2024

KristinAoki commented Aug 27, 2024

bradenmacdonald commented Aug 29, 2024

openedx-webhooks commented Aug 29, 2024

johnvente commented Jan 18, 2024 •

edited

Loading

openedx-webhooks commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

brian-smith-tcril commented Feb 15, 2024 •

edited

Loading