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

feat: improve types, close #970 #974

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@
// Using Method: Customize the first button
const toolbar = editor.getUI().getToolbar();

editor.eventManager.addEventType('clickCustomButton');
editor.eventManager.listen('clickCustomButton', function() {
editor.addEventType('clickCustomButton');
editor.on('clickCustomButton', function() {
alert('Click!');
});

Expand Down
25 changes: 20 additions & 5 deletions apps/editor/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ declare namespace toastui {
type HandlerFunc = (...args: any[]) => void;
type ReplacerFunc = (inputString: string) => string;
type CodeMirrorType = CodeMirror.EditorFromTextArea;
type CommandManagerExecFunc = (name: string, ...args: any[]) => any;
type CommandManagerExecFunc = (editor: Editor | MarkdownEditor | WysiwygEditor, ...args: any[]) => any;
type PopupTableUtils = LayerPopup;
type AddImageBlobHook = (fileOrBlob: File | Blob, callback: Function, source: string) => void;
type Plugin = (editor: Editor | Viewer, options: any) => void;
Expand Down Expand Up @@ -570,8 +570,19 @@ declare namespace toastui {
public styleToSelectedCells(onStyle: SquireExt, options?: object): void;
}

interface MarkdownRange {
from: CodeMirror.Position;
to: CodeMirror.Position;
ollapsed: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of this property is wrong, collapsed is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I will fix this

}

class CodeMirrorExt {
getCurrentRange(): MarkdownRange;
getEditor(): CodeMirrorType;
}

// @TODO: change toastMark type definition to @toast-ui/toastmark type file through importing
class MarkdownEditor {
class MarkdownEditor extends CodeMirrorExt {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add the CodeMirrorExt type to handle the event issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

static factory(
el: HTMLElement,
eventManager: EventManager,
Expand Down Expand Up @@ -695,7 +706,7 @@ declare namespace toastui {
class EventManager {
public addEventType(type: string): void;

public emit(eventName: string): any[];
public emit(eventName: string, ...args: any): any[];

public emitReduce(eventName: string, sourceText: string): string;

Expand Down Expand Up @@ -729,6 +740,12 @@ declare namespace toastui {

public addWidget(selection: Range, node: Node, style: string, offset?: number): void;

public addCommand(type: Command): void;

public addCommand(type: string, props: CommandPropsOptions): void;
Comment on lines +743 to +745
Copy link
Member

@seonim-ryu seonim-ryu May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you declare addCommand twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public addEventType(type: string): void;

public afterAddedCommand(): void;

public blur(): void;
Expand Down Expand Up @@ -835,8 +852,6 @@ declare namespace toastui {

public setMarkdown(markdown: string): void;

public setValue(markdown: string): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
It remains in the Editor class, can you remove it too?

public setValue(value: string, cursorToEnd?: boolean): void;


public setCodeBlockLanguages(languages?: string[]): void;
}
}
Expand Down
8 changes: 8 additions & 0 deletions apps/editor/src/js/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,14 @@ class ToastUIEditor {
}
}

/**
* Add event type when given event not exists
* @param {string} type Event type name
*/
addEventType(type) {
this.eventManager.addEventType(type);
}

/**
* After added command.
*/
Expand Down