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

fix #3, #4 and #5 #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix #3, #4 and #5 #6

wants to merge 1 commit into from

Conversation

avp90
Copy link

@avp90 avp90 commented Aug 27, 2021

fix #3, #4 and #5

@torrobinson
Copy link
Owner

Note to self @torrobinson :

  • Test updates on demo fiddle on main page
  • Test updates on personal JSS project

}
else {
// Add to the column
newRow.append(newCell);
Copy link
Owner

Choose a reason for hiding this comment

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

@avp90 the only issue I can foresee with this is that I had run newRow.append(newCell); BEFORE afterAdd handling because the user's custom afterAdd callback might want to climb up the newCell td and interact with the tr/row itself. I had wanted to only run a column's afterAdd after it was physically added to the row. If it is run any earlier, then it does not exist in the row yet and would be slightly more limited.

Was there a reason you moved it to after the callback handler?

Copy link
Author

@avp90 avp90 Sep 1, 2021

Choose a reason for hiding this comment

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

Yes sure it has a reason, but I don't know anymore why. 😄
Maybe to modify the newCell before add 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm if you're not sure of the reason, would you mind please putting the .append back before the afterAdd handler and check to see if your changes still work? I just already know of some personal usage that will break otherwise - unless it was required for your change

@torrobinson torrobinson added the enhancement New feature or request label Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow user to update cell value in afterChange
2 participants