feat: improve types, close #970 #974
Conversation
| interface MarkdownRange { | ||
| from: CodeMirror.Position; | ||
| to: CodeMirror.Position; | ||
| ollapsed: boolean; |
There was a problem hiding this comment.
The value of this property is wrong, collapsed is correct.
There was a problem hiding this comment.
good point, I will fix this
| public addCommand(type: Command): void; | ||
|
|
||
| public addCommand(type: string, props: CommandPropsOptions): void; |
There was a problem hiding this comment.
Why did you declare addCommand twice?
There was a problem hiding this comment.
The first type is different: the first one takes a single Command, the second one takes two parameters: a string and a CommandPropsOptions
https://github.com/nhn/tui.editor/blob/master/apps/editor/src/js/editor.js#L359-L365
I could declare it
public addCommand(type: string | Command, props?: CommandPropsOptions): void;
but I thought declaring the two options separately was more precise.
| @@ -835,8 +852,6 @@ declare namespace toastui { | |||
|
|
|||
| public setMarkdown(markdown: string): void; | |||
|
|
|||
| public setValue(markdown: string): void; | |||
There was a problem hiding this comment.
👍
It remains in the Editor class, can you remove it too?
tui.editor/apps/editor/index.d.ts
Line 802 in 7cddc9e
| // @TODO: change toastMark type definition to @toast-ui/toastmark type file through importing | ||
| class MarkdownEditor { | ||
| class MarkdownEditor extends CodeMirrorExt { |
There was a problem hiding this comment.
Why did you add the CodeMirrorExt type to handle the event issue?
There was a problem hiding this comment.
This one is actually a type bug fix. MarkdownEditor extends a CodeMirrorExt class:
https://github.com/nhn/tui.editor/blob/master/apps/editor/src/js/markdownEditor.js#L113
but this isn't represented in the types. I added two methods from CodeMirrorExt I found useful when developing custom commands: ideally I think they would all be added but this is quite a lot more work!
There was a problem hiding this comment.
@midgleyc I completed the review. Thank you for contributing! :)
But this PR will be difficult to merge. First, there are two issues in this PR. In this case, issues and PRs must be managed individually.
- Bug Fix : Remove
setValuefromindex.d.ts - Feature : Add new API (
addEventType)
And in case of the API that you want to add newly, it is better to handle it after the specification is decided as I answered in the previous issue.
So, it would be better to close this PR and remove setValue from index.d.ts to create the PR again. Removing setValue is simple and I can handle it, but it seems meaningful that you contribute. Then please check the comments with the above issue!
|
I have split this out into: #986 which contains only the setValue function removal Thanks for the review! |
Please check if the PR fulfills these requirements
fix #xxx[,#xxx], where "xxx" is the issue number)Description
As discussed in #970, I've found some types that I'd like to change. I've left each separate change in its own commit for easier discussion / reversion if necessary. I've tried to mark whether it was a fix (type was wrong) or feat (new public type).
setValue was present under the functions of Viewer, but calling it crashed the application as it doesn't exist any more.
When using custom events, you can pass additional arguments through emit. The first arg is always the name of the event, so I left it in.
eventManager wasn't available under Editor: I added a method to call-through for the method you need to call when adding a custom button, and updated the example to use it. I also noted that addCommand should be public: there is the same issue with commandManager.
CommandManagerExecFunc incorrectly said the first argument was a string -- I think it's actually one of Editor (for global commands), MarkdownEditor (for markdown commands) or WysiwygEditor (for wysiwyg commands).
Finally, there are some methods in MarkdownEditor that are accessible through CodeMirrorExt (that MarkdownEditor extends) that are useful when writing custom commands. I added the two I used.
Closes #970
Thank you for your contribution to TOAST UI product. 🎉 😘 ✨