-
Notifications
You must be signed in to change notification settings - Fork 79
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
EditorCore API extension #192
base: master
Are you sure you want to change the base?
Conversation
Why not just expose the |
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.
@proteye Wow! Thank you for your interest! 👋
|
||
deleteBlock(index?: number): Promise<void> | ||
|
||
setToFirstBlock(position: string, offset: number): Promise<boolean> |
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.
Could you provide use-case to me? Rather than exposing all API of editor-js, I think it would be better to identify use-case and apply the new abstraction 🙏🏻
@Jungwoo-An could you explain why you're against exposing the whole EditorJS api? EditorJS exposes their API for their reasons and this package is restricting it. In my opionion a wrapper should add minimal effort to make it compatible and leave all the heavy-lifting mechanics to the underlying package. I guess If you continue this way you'll end up maintaining every single low-level change and you might block and postpone adoption of new features because every single API change needs to be reflected in your wrapper resulting in more and more maintainance. What are your plans to avoid those scenarios? |
@christoph-kluge I agree. I don't see the point in just rewriting the EditorJS functionality into a layer library. |
It makes me wonder why this repository even hides the editor JS API in the
first place, it has gotten me in places where I've had to consider whether
we fork this, or implement our own react bridge for the original.
The biggest thing is the lack of react 18 support, where ReactDOM
deprecated the render call. That means this package has very little chance
of working after react 19.
But more on topic to this pull request, I support exposing the editor JS
API directly if possible.
…On Sun., May 8, 2022, 2:00 p.m. Ramil Zaynetdinov, ***@***.***> wrote:
@christoph-kluge <https://github.com/christoph-kluge> I agree. I don't
see the point in just rewriting the EditorJS functionality into a layer
library.
—
Reply to this email directly, view it on GitHub
<#192 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOSNPNTZU4KCLUKH6I3CV3VJATOVANCNFSM5UHXWCZA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@christoph-kluge @proteye @AlbinoGeek Thanks for good opinion. It was impressive. I'll change the API |
To leave my original thoughts behind, I want to implement the isomorphism of react-editor-js. I thought it could be rendered not only in DOM, but also in Node, react-native, other. I didn't think of react-editor-js as a simple wrapper of editor-js, I think it's a library that provides a higher level of API. However, as a library, I think it's a good idea to provide better DX than to maintain high-level APIs sometimes. Although direct access to editor-js is now restricted, we are trying to come up with a way to access APIs that are not abstracted from react-editor-js. (Maybe What do you think? @christoph-kluge @proteye @AlbinoGeek I will listen to many people's thoughts and create a new road map by reflecting them. |
I'm sorry for the late response. |
@proteye could you check if the It could be related to this issue. Thanks |
PR Type
Description
Core api extension: