Issues with typings in 2.1.0

[midgleyc]

I've been using this under Typescript, and I've found some issues with version 2.1.0.

1. Viewer does not have a "setValue" method. In version 1.4.10 it did, but it just called "setMarkdown".
2. If you're adding custom toolbar buttons, you need to add event types for your buttons to the eventManager. The examples do this as "editor.eventManager.addEventType('clickCustomButton');", but "eventManager" is not available in the index.d.ts. Is this the intended way of doing it? I can use "on" instead of "eventManager.listen", but the event type isn't registered.
3. Editor's "previewStyle" could be set to 'none' (or something other than the two allowed values) to not have a preview in the markdown. Null still works, and might be the intended way of doing things.

Would you accept a pull request to fix any of these, and which if yes?

[jack9603301]

In the latest version, setValue is replaced with setMarkdown

[jack9603301]

Consider using an updated API instead of an obsolete one!

[midgleyc]

I am doing. The issue is that it's still present in the index.d.ts, but it should not be.

[seonim-ryu]

@midgleyc

1. Viewer does not have a "setValue" method. In version 1.4.10 it did, but it just called "setMarkdown".
2. If you're adding custom toolbar buttons, you need to add event types for your buttons to the eventManager. The examples do this as "editor.eventManager.addEventType('clickCustomButton');", but "eventManager" is not available in the index.d.ts. Is this the intended way of doing it? I can use "on" instead of "eventManager.listen", but the event type isn't registered.
3. Editor's "previewStyle" could be set to 'none' (or something other than the two allowed values) to not have a preview in the markdown. Null still works, and might be the intended way of doing things.

For the first question, when I tested the declaration file(index.d.ts), it worked fine. Can you add a screenshot of how it comes out in your case?

tui.editor/apps/editor/index.d.ts

Line 836 in d0a1c47

public setMarkdown(markdown: string): void;

tui.editor/apps/editor/test/types/type-tests.ts

Line 152 in d0a1c47

viewer.setMarkdown('### I am Viewer!');

And for the second question, that's a bug. However, I am suddenly wondering if it is right to use it as editor.eventManager. In this case, the user accesses the private property assigned inside the instance. I think it is wrong to use it. Instead, add the method API that returns eventManager and change it by calling that API. I think it's not just a Type's problem.

For the last question, the previewStyle option value can only use 'tab' or 'vertical'. I think this problem was discovered by accident, disabling the preview area by setting 'none' or other value to the previewStyle option. But this should be considered usability. Have you ever used the Markdown editor without a preview area? For what reason? And how do you know the preview disappears when set to 'none'? 🤔

Would you accept a pull request to fix any of these, and which if yes?

I think this means you're sending PR (Pull Request). Contributing is always welcome, so you can do that! Then I will wait for your answer. 😃

[midgleyc]

Thanks for the response!

For 1., the problem is the opposite way around: the index.d.ts file specifies a setValue method:

tui.editor/apps/editor/index.d.ts

Line 838 in d0a1c47

public setValue(markdown: string): void;

but trying to call this method fails because it doesn't actually exist.

For 2. I agree -- there is an "addCommand" method that allows you to access the method on CommandManager you need (though it isn't in the index.d.ts) -- I think a nice solution is to add an "addEventType" that calls through to eventManager.

Alternatively, "on" could be changed to add an event if it hasn't been added yet (because if you don't add an event before listening for it, the program will crash), but I can't see a way to tell this through the public API.

For 3., I've inherited a project that has no preview area for the Markdown. I can try to check why when the Product Owner comes back on Monday. It might be because you can flip to the WYSIWYG tab to see what it looks like, so the PO decided that the preview mode was unnecessary noise.

If you set the previewStyle to 'none', you can see that the preview disappears 😉. Null is also an allowed value, and does work even if it's not supposed to be supported:

tui.editor/apps/editor/index.d.ts

Line 234 in d0a1c47

previewStyle?: PreviewStyle;

[seonim-ryu]

@midgleyc Sorry, the answer is late.

For 1. As of the answer above, since version 2.0, the setValue method is deprecated and changed to using setMarkdown, setHtml. The type of setValue method should be removed from index.d.ts, but it is missing while processing. 😅

#970 (comment)

For 2 and 3. These two are related to the specification, so we are discussing these issues.

For example, when using eventManager, a problem occurs when using a custom toolbar. The way I thought was to add a method API that can get the eventManager, but there is also a way to handle the event so that it can be registered through the option of adding the toolbar's item API (toolbar.insertItem()). I think it should be decided considering these various methods.

So the correct answer will be given once these discussions are decided. I would appreciate it if you wait a little longer! 🙏

[midgleyc]

but there is also a way to handle the event so that it can be registered through the option of adding the toolbar's item API (toolbar.insertItem())

You don't need to use insertItem to insert a new button. The Editor constructor takers a toolbarItems parameter that you can pass custom buttons to:

https://github.com/nhn/tui.editor/blob/master/apps/editor/src/js/editor.js#L141

If the constructor would go through our custom buttons and register our events itself, that would also be nice, but having insertItem alone would mean we'd have to rewrite this, and I prefer the way that the up-front declaration works.