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

Make Column scroll position usefull #6083

Open
hyamanieu opened this issue Dec 18, 2023 · 4 comments · May be fixed by #7206
Open

Make Column scroll position usefull #6083

hyamanieu opened this issue Dec 18, 2023 · 4 comments · May be fixed by #7206
Labels
type: enhancement Minor feature or improvement to an existing feature

Comments

@hyamanieu
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

I find the scroll_position parameter to be only useful for setting it to 0, but not for jumping to a specific area. I naively thought the integer represented the index of the listed objects, but it corresponds to the scrollTop property.

I want a method to programmatically jump to an element of the list.

In general, I believe it can be usefull for other subclass of ListPanel, like ChatFeed, as long as a scroll bar may appear.

Describe the solution you'd like

I can see two very different approaches with pros and cons:

  1. Implement a focus method on python side which does the same as HTMLElement.focus({ preventScroll: false }). Then it's not about modifying ListPanel but about adding this method to all Viewable objects.
  • pros: it responds directly to the problem
  • cons:
    • it may be more complex to implement.
    • it changes the focus (of course), which may be an unwanted side-effect unrelated to the original issue (not my case)
  1. Modify column.ts so that ScrollHeight is a model property as well, helping with calculating a percentage of scrolling. (pros & cons are swapped)

Describe alternatives you've considered

  1. Regarding my other issues Panel objects should mirror bokehjs convention #2834, closed PR Proposal to fix #2834 #2842, and now Accessing bokeh models #6073, I was able to find a specific object by setting a name or tags, then calling the .focus() method. However it doesn't work anymore in Panel 1.
  2. I can still find the column object by calling its specific custom class, then getting its scrollHeight. Then just estimating where I should be at. It's not accurate.
  3. like 2, but calling for .shadowRoot.children[X].focus(), however I have no idea how to get child X.
  4. like 2, but setting as many custom CSS class as there are children 🙄

Additional context

I'm willing to work on a PR if given a direction either among my two approaches or a third one I have missed.

@hyamanieu hyamanieu added the TRIAGE Default label for untriaged issues label Dec 18, 2023
@ahuang11
Copy link
Contributor

Thanks for sharing this!

I think scroll_position is already a public parameter so if we change it, it'd be a breaking change, but we should definitely update the docs to mention it corresponds to the scrollTop property.

So, to add to your proposal, I imagine:

  1. create a new parameter called scroll_index which scrolls to the index of the listed objects
  2. new method called scroll_to_index() which implements the functionality somewhere here
  3. That sets scroll_position

For reference, this might be helpful to understand the internals if you're contributing a PR!
#5746 (comment)

(I think working internally on the models is much simpler than working externally; the children are directly accessible thru the model rather than using querySelector & etc)

Also, happy to answer questions you may have on the way!

@hyamanieu
Copy link
Collaborator Author

hyamanieu commented Dec 18, 2023

Thanks for your feedback. With your proposal (which is better than mines), it means we would need to sync the scroll_index parameter as well, it's not only about setting the position. Otherwise the parameter would be systematically off.

I would guess checking which child has the closest, strictly smaller .getBoundingClientRect().top compared to the parent Column every time "scroll_position" is changed.

Something along the line of adding the line this.on_change(scroll_position, () => this.set_scroll_index()); to connect_signals, then

set_scroll_index(): void {
    this.model.scroll_index= Math.round(<finding index of first child with top directly smaller than column top>);
  }

What do you think?

@ahuang11
Copy link
Contributor

Yes I believe that could work!

@hyamanieu
Copy link
Collaborator Author

Although still relevant for pn.Column, this issue is linked to a list of ChatMessage, so before I would start working on pn.Column which is then not use for other ListLike (like ChatFeed), I would like your feedback to my latest message, @ahuang11 :

#6021 (comment)

@ahuang11 ahuang11 added type: enhancement Minor feature or improvement to an existing feature and removed TRIAGE Default label for untriaged issues labels Jan 19, 2024
@ahuang11 ahuang11 linked a pull request Aug 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants