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

Feature possibilities to merge blocks of different types #2564

Conversation

GuillaumeOnepilot
Copy link
Contributor

@GuillaumeOnepilot GuillaumeOnepilot commented Dec 16, 2023

For some context, I have a need for potential custom blocks to accept other different types of blocks to be merged in them (think Notion behavior), here is an example from Notion:

Screen.Recording.2023-12-16.at.21.51.38.mov

My solution is to add a new static property which contains the block's type that can be merged inside the target block, I also added the type to the merge functions in case some blocks would want to make conditional merging.

Here is the result of my implementation (which I agree is a bit weird, in some sense because who would want to merge a paragraph into a title, but who knows):

Screen.Recording.2023-12-16.at.21.56.30.mov

and with my specific use case (custom blocks):

Screen.Recording.2023-12-16.at.22.02.21.mov

@maxheyn
Copy link

maxheyn commented Dec 22, 2023

I like the idea of this feature.

@GuillaumeOnepilot
Copy link
Contributor Author

Hey @neSpecc, I updated the code to use the conversion config (this make a lot of sense in fact), should I add test cases for this?

@neSpecc
Copy link
Member

neSpecc commented Dec 23, 2023

Hey @neSpecc, I updated the code to use the conversion config (this make a lot of sense in fact), should I add test cases for this?

Cool. Yes, tests are needed

@GuillaumeOnepilot
Copy link
Contributor Author

GuillaumeOnepilot commented Dec 24, 2023

@neSpecc I added only one test, but I don't see what more I can test 🤔

On another note, I think this feature could be considered breaking changes for some users, maybe a flag on the editor config should be toggled to make it work just in case?

@GuillaumeOnepilot
Copy link
Contributor Author

@neSpecc Hey any update on this PR?

asfixia added a commit to asfixia/editor.js that referenced this pull request Feb 24, 2024
@GuillaumeOnepilot
Copy link
Contributor Author

@neSpecc I've made the changes, and also updated the change log

@neSpecc
Copy link
Member

neSpecc commented Mar 23, 2024

Merging is working but I see problems with Caret. It should stay in a glue-place.

Seem like removing requestAnimationFrame here makes it better:

image

But there are still issues with saving a caret position. Need to debug it.

@GuillaumeOnepilot
Copy link
Contributor Author

GuillaumeOnepilot commented Mar 23, 2024

Merging is working but I see problems with Caret. It should stay in a glue-place.

Seem like removing requestAnimationFrame here makes it better:

But there are still issues with saving a caret position. Need to debug it.

Can you give me some indication on how to reproduce the issue, on my side, the caret seems to be at the right place

Screen.Recording.2024-03-23.at.23.17.50.mov

@neSpecc
Copy link
Member

neSpecc commented Mar 24, 2024

Can you give me some indication on how to reproduce the issue, on my side, the caret seems to be at the right place

Try merging blocks of different types. For example header -> text, text -> text

@neSpecc
Copy link
Member

neSpecc commented Mar 30, 2024

Hey, @GuillaumeOnepilot. Would you open your branch for pushing? I have resolved issues with caret (Safari-related): partially in Paragraph tool editor-js/paragraph#86, partially in editor.

Also, we noticed that there was no sanitising on merge, so I've added it as well.

@GuillaumeOnepilot
Copy link
Contributor Author

@neSpecc For some reason, the allow edit from maintainer checkbox just doesn't show up
Screenshot 2024-03-30 at 20 04 20

@neSpecc
Copy link
Member

neSpecc commented Mar 30, 2024

@neSpecc For some reason, the allow edit from maintainer checkbox just doesn't show up

It's strange. Do you have enough rights in the fork repo?

@GuillaumeOnepilot
Copy link
Contributor Author

GuillaumeOnepilot commented Mar 30, 2024

@neSpecc For some reason, the allow edit from maintainer checkbox just doesn't show up

It's strange. Do you have enough rights in the fork repo?

I made the fork but it's under my company organisation, let me check but I'm pretty sure that I'm a admin

@GuillaumeOnepilot
Copy link
Contributor Author

I don't see anything on the fork that could help me, do you know by any chance what should I change ?

@GuillaumeOnepilot
Copy link
Contributor Author

Oh from what I read on the GitHub docs it seems related to the ownership of the repo 🤔

My Fork is technically not mine and it's not a personal account

Screenshot 2024-03-30 at 20 47 27

@neSpecc
Copy link
Member

neSpecc commented Mar 30, 2024

Oh from what I read on the GitHub docs it seems related to the ownership of the repo 🤔

My Fork is technically not mine and it's not a personal account

I've recreated a PR based on your commits with adding mine.

#2671

@neSpecc
Copy link
Member

neSpecc commented Apr 1, 2024

Shipped by #2671

@neSpecc neSpecc closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants