Skip to content

comments#1

Open
ayal wants to merge 1 commit into
mainfrom
comments-1
Open

comments#1
ayal wants to merge 1 commit into
mainfrom
comments-1

Conversation

@ayal
Copy link
Copy Markdown
Collaborator

@ayal ayal commented Mar 3, 2022

No description provided.

@ayal ayal requested a review from davidvaks March 3, 2022 13:01
Comment thread vanilla/todo.js
Comment thread vanilla/todo.js
Comment thread vanilla/todo.js
Comment on lines +48 to +51
// i'm not sure path is encouraged or standard, and even if it was, it's too fragile
// what if you changed the html? this would break. maybe use "Element.closest" ?
const row = event.path[2];
const taskTextCell = row.cells[1];
const taskTextCell = row.cells[1]; // this is fragile too
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

yes, this is one of the places I really didn't like.
tried doing
event.target.parentNode() like in the MDN but got an exception for parentNode not being a function...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

so yeah closest should work.

Comment thread vanilla/todo.js
Comment thread vanilla/todo.js
Comment thread vanilla/todo.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants