Skip to content

Update targetjs #1829

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

Merged
merged 7 commits into from
Feb 11, 2025
Merged

Update targetjs #1829

merged 7 commits into from
Feb 11, 2025

Conversation

wasfi2
Copy link
Contributor

@wasfi2 wasfi2 commented Jan 29, 2025

This is a new implementation of TargetJS. It is a lighter implementation than the previous one. I also now use the click event instead of pointer up events.

@krausest krausest merged commit d6565f1 into krausest:master Feb 11, 2025
@wasfi2 wasfi2 deleted the update-targetjs branch February 11, 2025 19:08
@krausest
Copy link
Owner

Thanks a lot, well worth the wait. Here's a screenshot from the result:
Screenshot 2025-02-11 at 20 57 51

@krausest
Copy link
Owner

It seems appropriate to me to add note #772, #800 and #801 to your implementation. Would you agree?
(Shouldn't be meant as a critique. It seems to me that this is the way it's supposed to work for your framework and not for cutting corners.)

@wasfi2
Copy link
Contributor Author

wasfi2 commented Feb 11, 2025

I agree with #772.
#800 I am not sure if I understand it. Can you please elaborate more?
#801. Events are not directly tied to the DOM. They trigger a task cycle process. The process then collects all the events happen before it runs, so the events can be handled synchronously by the visible models.

@krausest
Copy link
Owner

If an implemenation moves the selection state to the row (instead of the table) then it's a candidate for #800
But I think you're right. #772 might be enough - the selected class is directly set on the node.
So I'll just add #772.

@wasfi2
Copy link
Contributor Author

wasfi2 commented Feb 11, 2025

Sounds good. Thanks for clarifying.

@wasfi2
Copy link
Contributor Author

wasfi2 commented Feb 12, 2025

I noticed the results have been published. Thanks a lot for getting it done so quickly!

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