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

EditorCore API extension #192

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

proteye
Copy link

@proteye proteye commented Apr 25, 2022

PR Type

  • Bug Fix
  • Feature Request

Description

Core api extension:

  • toggleReadOnly(): Promise
  • insertBlock(type?: string, data?: BlockToolData, config?: ToolConfig, index?: number, needToFocus?: boolean): Promise
  • updateBlock(id?: string, data?: BlockToolData): Promise
  • deleteBlock(index?: number): Promise
  • setToFirstBlock(position: string, offset: number): Promise
  • setToLastBlock(position: string, offset: number): Promise
  • setToBlock(index: number, position: string, offset: number): Promise
  • focus(atEnd: boolean): Promise
  • openToolbar(): Promise
  • closeToolbar(): Promise

@AlbinoGeek
Copy link

Why not just expose the API from @editorjs/editorjs directly, any particular reason?

@Jungwoo-An Jungwoo-An self-requested a review May 8, 2022 10:59
Copy link
Owner

@Jungwoo-An Jungwoo-An left a 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>
Copy link
Owner

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 Jungwoo-An added the enhancement New feature or request label May 8, 2022
@christoph-kluge
Copy link

@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?

@proteye
Copy link
Author

proteye commented May 8, 2022

@christoph-kluge I agree. I don't see the point in just rewriting the EditorJS functionality into a layer library.

@AlbinoGeek
Copy link

AlbinoGeek commented May 8, 2022 via email

@Jungwoo-An
Copy link
Owner

@christoph-kluge @proteye @AlbinoGeek Thanks for good opinion. It was impressive. I'll change the API

@Jungwoo-An
Copy link
Owner

Jungwoo-An commented Jun 4, 2022

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 editorCore.dangerouslyLowLevelInstance

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.

@Jungwoo-An
Copy link
Owner

I'm sorry for the late response.

@SalahAdDin
Copy link

@proteye could you check if the i18nAPI is being omitted by this package so you could include it?

It could be related to this issue.

Thanks

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

Successfully merging this pull request may close these issues.

None yet

5 participants