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

Add a check that allows inline toolboxes to work with nested editorsjs #2581

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

loucass003
Copy link

In our project, we have cases where we can have nested editorjs.
For instance, we have a block tool that will create an editorjs instance inside of it.

We ran into a situation where the inline toolbox would close when opening the link tool inside a nested editor.
This is due to the editor checking for the parent block only to verify if we clicked outside the toolbox.

We added a check that should prevent this issue (at least it does for us) and also keep the standard functionality of editorjs.
We hope that this change can be merged so we can get rid of our fork and make everyone else benefit from it.

Respectfully, Lucas Lelievre

@2tanhamon
Copy link

Hello @loucass003, i also have nested EditorJs and also have the same problem. But i have and another broblem with nested Editor js, maybe did u meet the same problem too? #2585

@loucass003
Copy link
Author

Hello @loucass003, i also have nested EditorJs and also have the same problem. But i have and another broblem with nested Editor js, maybe did u meet the same problem too? #2585

Sorry we have not. We do not use react for our project maybe something related with that?

@calumk
Copy link

calumk commented Mar 1, 2024

It would be really good if this could be merged - It would fix an ongoing issue with a popular plugin :

calumk/editorjs-columns#3

@sureshHARDIYA
Copy link

Could we please prioritize this?

Copy link
Member

Choose a reason for hiding this comment

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

please, remove unrelated changes

*/
const clickedOutsideBlockContent = focusedElement.closest(`.${Block.CSS.content}`) === null;
const closestBlock = focusedElement.closest(`.${Block.CSS.content}`);
const clickedOutsideBlockContent =
Copy link
Member

Choose a reason for hiding this comment

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

add corresponding test case

@vamidi
Copy link

vamidi commented Mar 18, 2024

Would love to see this fixed =D

@calumk
Copy link

calumk commented Mar 22, 2024

@loucass003 do you have time to write a test for this and submit a clean PR?

@neSpecc I can submit a clean PR but im not familiar with cypress so would probably take me a while to work out how to write the test for this

@calumk
Copy link

calumk commented Apr 4, 2024

@loucass003 - sorry to bump, but as you originally proposed this, do you have time to write a test for this and submit a clean PR?

@perssonrichard
Copy link

We're also working with nested EditorJS instances so this would be more than great.

@calumk
Copy link

calumk commented Apr 8, 2024

Hi @neSpecc - Can you please provide some guidance as to how exactly to test for this?

The issue as i see it is this specific feature (nested editor.js) doesnt exist in any of the default plugins.

So to test with cypress, unless I misunderstand, we would need to add a 3rd-party example plugin to the package devDependencies

This might be something you are unwilling to merge

Either we would need to create a new plugin to test the simpleist intergration, or use an exisiting one, such as my own @calumk/editor-js-columns

Thoughts?

@gorenburg
Copy link

this one is blocker for us as well. what approach should be used for the tests?

@neSpecc
Copy link
Member

neSpecc commented May 4, 2024

What about testing, you can create a Tool Mock what will render editor.js inside own render method. Example https://github.com/codex-team/editor.js/blob/next/test/cypress/tests/modules/BlockEvents/Backspace.cy.ts#L66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants