Open
Conversation
neSpecc
reviewed
Jun 12, 2024
| * @property {boolean} checked - is the item checked | ||
| */ | ||
| import './polyfills.js'; | ||
| interface ChecklistItem { |
| * @typedef {object} ChecklistData | ||
| * @property {ChecklistItem[]} items - checklist elements | ||
| */ | ||
| interface ChecklistData { |
| "scripts": { | ||
| "dev": "vite", | ||
| "build": "vite build" | ||
| "build": "tsc && vite build" |
Contributor
There was a problem hiding this comment.
vite will automatically compile the .ts file, isn't it?
Comment on lines
+34
to
+37
| private _elements: { wrapper: HTMLElement | null; items: HTMLElement[] }; | ||
| private readOnly: boolean; | ||
| private api: any; | ||
| private data: ChecklistData; |
Contributor
There was a problem hiding this comment.
please, add docs for all interfaces and properties
Comment on lines
+7
to
+9
| * @typedef {object} ChecklistItem | ||
| * @property {string} text - item label | ||
| * @property {boolean} checked - is the item checked |
Contributor
There was a problem hiding this comment.
move docs to each property, please
Contributor
There was a problem hiding this comment.
still actual. Typedef is not needed anymore, use comments near properties instead
|
|
||
| const items = this.items; | ||
| const currentItem = document.activeElement.closest(`.${this.CSS.item}`); | ||
| const currentItem = document.activeElement?.closest(`.${this.CSS.item}`) as HTMLElement; |
| backspace(event) { | ||
| const currentItem = event.target.closest(`.${this.CSS.item}`); | ||
| backspace(event: KeyboardEvent): void { | ||
| const currentItem = (event.target as HTMLElement).closest(`.${this.CSS.item}`) as HTMLElement; |
| } | ||
|
|
||
| const selection = window.getSelection(); | ||
| const selection = window.getSelection() as Selection; |
Comment on lines
+1
to
+5
| /** | ||
| * Remove and return HTML content after carer position in current input | ||
| * | ||
| * @returns {DocumentFragment} extracted HTML nodes | ||
| */ | ||
| export function extractContentAfterCaret() { | ||
| const input = document.activeElement; | ||
| const selection = window.getSelection(); | ||
| * Remove and return HTML content after carer position in current input | ||
| * | ||
| * @returns {DocumentFragment} extracted HTML nodes | ||
| */ |
Comment on lines
+7
to
+8
| const input = document.activeElement as HTMLElement; | ||
| const selection = window.getSelection() as Selection; |
arenac
force-pushed
the
typescript-support
branch
2 times, most recently
from
da57ad8 to
6d8a249
Compare
arenac
force-pushed
the
typescript-support
branch
from
6d8a249 to
9c848ec
Compare
Author
|
@neSpecc thanks for the review so far. I pushed some udpates. |
neSpecc
reviewed
Jun 17, 2024
Comment on lines
+7
to
+9
| * @typedef {object} ChecklistItem | ||
| * @property {string} text - item label | ||
| * @property {boolean} checked - is the item checked |
Contributor
There was a problem hiding this comment.
still actual. Typedef is not needed anymore, use comments near properties instead
| * @returns {{export: Function, import: Function}} | ||
| */ | ||
| static get conversionConfig() { | ||
| static get conversionConfig(): { export: (data: ChecklistData) => string; import: (str: string) => ChecklistData } { |
Contributor
There was a problem hiding this comment.
show me the error you got. I think, it should work fine. Maybe you need to make ChecklistData extend BlockToolData ?
| if (isLastItem) { | ||
| const currentItemText = getHTML(this.getItemInput(currentItem)); | ||
| const isEmptyItem = currentItemText.length === 0; | ||
| if (isLastItem && currentItem) { |
Contributor
There was a problem hiding this comment.
this change can break the logic. Its better to handle currentItem=undefined case separately above
| backspace(event) { | ||
| const currentItem = event.target.closest(`.${this.CSS.item}`); | ||
| backspace(event: KeyboardEvent): void { | ||
| const currentItem = (event.target as HTMLElement).closest(`.${this.CSS.item}`) as HTMLElement; |
neSpecc
requested changes
Jun 19, 2024
| * Checklist Tool for the Editor.js 2.0 | ||
| */ | ||
| export default class Checklist { | ||
| /** @private HTML nodes */ |
Contributor
There was a problem hiding this comment.
@private is not needed in typescript since you explicitly specify access modifier
| { | ||
| "name": "@editorjs/checklist", | ||
| "version": "1.6.0", | ||
| "version": "1.7.0", |
Contributor
There was a problem hiding this comment.
Suggested change
| "version": "1.7.0", | |
| "version": "1.6.1", |
| * @property {boolean} checked - is the item checked | ||
| * CheckList constructor options | ||
| */ | ||
| export interface ChecklistOptions { |
Contributor
There was a problem hiding this comment.
lets use the BlockToolConstructorOptions the from editorjs package
| * @returns {{icon: string, title: string}} | ||
| */ | ||
| static get toolbox() { | ||
| static get toolbox(): { icon: string; title: string } { |
Contributor
There was a problem hiding this comment.
Suggested change
| static get toolbox(): { icon: string; title: string } { | |
| static get toolbox(): ToolboxConfig { |
| /** | ||
| * Checklist Tool for the Editor.js 2.0 | ||
| */ | ||
| export default class Checklist { |
Contributor
There was a problem hiding this comment.
Suggested change
| export default class Checklist { | |
| export default class Checklist implements BlockTool { |
| checkListItem.classList.toggle(this.CSS.itemChecked); | ||
| checkbox.classList.add(this.CSS.noHover); | ||
| checkbox.addEventListener('mouseleave', () => this.removeSpecialHoverBehavior(checkbox), { once: true }); | ||
| toggleCheckbox(target: HTMLElement | null, event: MouseEvent): void { |
Contributor
There was a problem hiding this comment.
why target has been added?
| export function moveCaret(element: HTMLElement, toStart: boolean = false, offset?: number): void { | ||
| const range = document.createRange(); | ||
| const selection = window.getSelection(); | ||
| const selection = window.getSelection() as Selection; |
Contributor
There was a problem hiding this comment.
casting is not expected here, it can lead errors since there can be no selection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding Typescript support to improve developement experience.
And generating types declarion during build, to be published and also to improve development experirce for the library users.