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

Add onselect #209

Closed
wants to merge 2 commits into from
Closed

Add onselect #209

wants to merge 2 commits into from

Conversation

iliakap
Copy link

@iliakap iliakap commented Dec 23, 2022

No description provided.

@josdejong
Copy link
Owner

Thanks @iliakap for your PR, I'll look into it after the holidays

@josdejong
Copy link
Owner

Thanks again for your PR @iliakap .

For reference: this PR will resolve #163.

Two questions:

  1. Right now, the onSelect will work for tree and table mode, but not for text mode. I think we need to at least have a clear plan on how to implement support for selection for text mode: either a different method, or via an extra parameter or so. What are your thoughts on that?
  2. Besides listening for changes in the selection, we'll also need a way to change the selection, like I describe in Feature-Request: onSelect #163 (comment). What do you think?

@josdejong
Copy link
Owner

Closing this PR now due to lack of response. Feel free to reopen if needed.

@josdejong josdejong closed this Feb 23, 2023
@iliakap
Copy link
Author

iliakap commented Mar 13, 2023

@josdejong, just saw ur comments, sorry for the late response :)
I wont reopen now since I'll only have time to look at it in a few weeks.
with regards to your questions:

  1. having hard time to remember, but I recall the text mode is just free text, I think it can be implemented using the web selection api, but it felt out of scope.
  2. I agree it's a nice addition, LMK if you wanna add this to this PR, not sure what the timeline will be though :)

@josdejong
Copy link
Owner

Thanks for getting back @iliakap, no problem at all. Let's continue the discussion in #163

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