-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: next
Are you sure you want to change the base?
Conversation
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? |
It would be really good if this could be merged - It would fix an ongoing issue with a popular plugin : |
Could we please prioritize this? |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add corresponding test case
Would love to see this fixed =D |
@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 |
@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? |
We're also working with nested EditorJS instances so this would be more than great. |
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? |
this one is blocker for us as well. what approach should be used for the tests? |
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 |
created new PR to fix this issue with tests: #2780 |
@gorenburg amazing 🤩 |
@neSpecc Are the tests @gorenburg provided sufficiant? |
This PR can be closed since it was fixed with #2780 |
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